Fix BatchCryst concentration Jacobian sign#115
Conversation
parkyr
left a comment
parkyr
left a comment
There was a problem hiding this comment.
Maintainer review. Note: I am the PR author, so this account cannot submit APPROVE/REQUEST_CHANGES; posting as COMMENT. There are no blocking findings.
Verdict
The sign fix is mathematically correct and the regression test genuinely guards it. I would consider this merge-ready pending a formal approval from another maintainer if branch protection requires one.
Correctness — confirmed
The material balance is dc_i/dt = -transf/vol * (kron_jtg_i - c_i/rho_l) (material_balances, Crystallizers.py:1513, with transf == tr). Differentiating the explicit -c_i/rho_l term w.r.t. c_i (holding tr fixed) yields +tr/(vol*rho_l). In the code's assembled form dfconc_dconc = -1/vol_liq * (dtr_dconc_tg*first_conc + second_conc), recovering that positive diagonal requires second_conc = -tr/rho_l * I. The prior +tr/rho_l inverted the self-derivative sign for every species. The change matches the differentiated balance and the issue #26 analysis.
Test — genuinely red-on-base, green-on-head
Verified locally against the .venv:
tests/test_batch_cryst_concentration_jacobian.pyfails on base1a3ab31(Max relative difference: 2.0, i.e. the flipped diagonal) and passes on headd30bc6a.- The assimulo-module stubbing lets it collect in the minimal env; marked
unit, which is registered under--strict-markers.
Scope — narrow scope is correct
BatchCryst is the only crystallizer class with an analytic jac_states; MSMPR/SemibatchCryst inherit jac_states_numerical, so no sibling carries the same second_conc sign bug. Closes #26 is accurate — this fully resolves the diagonal-sign defect described in the issue.
Findings
Nonblocking The test recomputes tr, dtr_dconc_tg, and first_conc with the same expressions as production, so it independently pins only the sign of the diagonal (-tr/rho_l) — an error in dtr_dconc_tg or first_conc would not be caught because both sides would move together. This is acceptable for a targeted sign regression since the discriminating -tr/rho_l is written explicitly, but a hand-computed literal for the full expected block (or an independent finite-difference check of the Jacobian against unit_model) would make it robust to the other terms. Crystallizers.py:1405 / test lines 108-115.
Review summary
- Blocking: none.
- Nonblocking: 1 (test independence, above).
- Questions: none.
- Tests run:
pytest tests/test_batch_cryst_concentration_jacobian.py— red on base1a3ab31, green on headd30bc6a;pytest tests/ -m "not assimulo"— 34 passed, 8 deselected. Run in the repo.venvagainst a clean worktree at the PR head. - Linked issue: #26 — fully resolved;
Closes #26is correct. - GitHub checks (head
d30bc6a): Core tests SUCCESS, Assimulo integration tests SUCCESS. - Merge readiness:
mergeStateStatusCLEAN. PR body notes a same-file/non-same-hunk overlap with PR #106; rebase if #106 lands first.
|
Addressed the review-body nonblocking finding Commits pushed:
Changes made:
Tests run:
CI on
Not addressed:
Remaining gates:
|
|
|
||
| first_conc = np.outer(self.kron_jtg - w_conc/rho_l, self.kron_jtg) | ||
| second_conc = tr/rho_l * np.eye(len(w_conc)) | ||
| second_conc = -tr/rho_l * np.eye(len(w_conc)) |
There was a problem hiding this comment.
Units in this line and all across this file would be really useful. Let’s add them now that we can
There was a problem hiding this comment.
Thanks for the review. I noticed that similar comments about units and documentation have come up across several issues and PRs. It may be more effective to consolidate those broader concerns into a dedicated epic issue (and address it in a separate PR), allowing each PR review to stay focused on the specific issue being addressed. what do you think? we can also discuss the strategy forward in the next offline discussion.
There was a problem hiding this comment.
I think that we can have a single documentation issue indeed, but as we work in all the files, we should slowly add these. Also, the unit analysis allows for debugging
2 participants
Summary
Closes #26.
BatchCryst.jac_statesconcentration Jacobian self-derivative sign so it matches the differentiated material balance.Triage And Acceptance Criteria
Triage comment: #26 (comment)
This PR satisfies the recorded criteria:
BatchCryst.jac_states(..., return_only=False)uses the differentiated material-balance sign for the concentration self-derivative term.1a3ab3155216f07e9a18c60687a7b6b1c5959202and passes with this fix.Local Checks
/home/yirangpark/repos/secquoia/PharmaPy/.venv/bin/python -m pytest tests/test_batch_cryst_concentration_jacobian.py -q/home/yirangpark/repos/secquoia/PharmaPy/.venv/bin/python -m pytest tests/test_batch_cryst_concentration_jacobian.py -q/home/yirangpark/repos/secquoia/PharmaPy/.venv/bin/python -m pytest tests/ -m "not assimulo" -qgit -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol diff --cached --checkBranch Hygiene
masterorigin/masterat1a3ab3155216f07e9a18c60687a7b6b1c5959202upstream/masteris contained inorigin/masterPharmaPy/Crystallizers.py, but its current hunks are in crystallizer energy/jacket paths around lines 590, 1874, and 2216. This PR touches theBatchCryst.jac_statesconcentration block around line 1402, so the current overlap is same-file but not same-hunk. If PR Fix crystallizer energy balance units and jacket paths #106 lands first, rebase this branch on the updatedmasterbefore merging.