Fix crystallizer energy balance units and jacket paths#106
Conversation
andres9403
left a comment
andres9403
left a comment
There was a problem hiding this comment.
Maintainer review (posted as COMMENT because this account (andres9403) is the PR author; GitHub rejects author APPROVE/REQUEST_CHANGES). No blocking findings.
The PR resolves all four defects in #68, and each is locked by a test in the new tests/test_crystallizer_energy_balances.py. I verified the four tests fail against the pre-fix source (checked out the merge-base versions of Crystallizers.py/MixedPhases.py) and pass on the head, so they are not vacuously green:
- Defect 1 (adiabatic MSMPR):
energy_balancesnow short-circuits withht_term = 0and returns the scalardtemp_dtwhenself.adiabatic or temp_ht is None, sounit_model'snp.appendyields the singletempderivative matchingstates_uo. No jacket dereference oftemp_ht=None. - Defect 2 (Semibatch jacket): reads
temp_in/vol_flowfromself.Utility.get_inputs(time)andcp/rhofromUtility, dropping the never-assignedself.temp_ht_in/self.flow_ht. Mirrors the MSMPR pattern. - Defect 3 (liquid-feed enthalpy basis): the plain-liquid branch now multiplies specific enthalpy by
rho_liq_in, soh_inis volumetric (J/m^3), consistent with the volumetrich_spin both MSMPR (flow_term = input_flow*(h_in - h_sp)) and Semibatch (flow_term = input_flow*h_inwithaccum_termin J/s). - Defect 4 (
Slurry.getCpmutation):volfracsis copied before thetimes_vliqscaling, so the caller's list is no longer mutated;self.epsilon/self.densitiesare write-only elsewhere, so the list->ndarray type change is inert.
One non-blocking Question is left inline on the source_term annotation. See the summary below.
| source_term = dh_cryst*cryst_rate * vol | ||
| # Energy terms | ||
| flow_term = input_flow * (h_in - h_sp) # [J/s] | ||
| source_term = dh_cryst*cryst_rate * vol # [J/s] |
There was a problem hiding this comment.
Question (non-blocking): this line is annotated # [J/s] for dh_cryst*cryst_rate * vol, but the analogous SemibatchCryst.energy_balances annotates source_term = dh_cryst * cryst_rate (no * vol) also as # [J/s]. Both can only be [J/s] if cryst_rate (the transf returned by material_balances) carries different units in the two classes ([kg/m**3/s] here vs [kg/s] in Semibatch). That is pre-existing numeric code, not changed by this PR, so it is not blocking. But since #68 is about basis/unit consistency in exactly these balances, could you confirm the two cryst_rate bases and annotate cryst_rate at the material_balances boundary so the [J/s] labels are unambiguous?
There was a problem hiding this comment.
Addressed in 57dd0d2: annotated the material_balances return basis for transf and the paired source_term comments. MSMPR uses cryst_rate [kg/m**3/s] and multiplies by vol; Batch/Semibatch use cryst_rate [kg/s].
| rho_ht = 1000 | ||
| vol_ht = self.vol_tank*0.14 # m**3 | ||
| ht_media = self.Utility.get_inputs(time) | ||
| tht_in = ht_media['temp_in'] # [K] |
There was a problem hiding this comment.
What did it changed from "degC" to "#[K]" are we sure about the input units of ht_media['temp_in']
There was a problem hiding this comment.
This still seems to be an issue. Let’s please resolve it
There was a problem hiding this comment.
Yes, this happened since in the first commit i asked to add units to the touched functions. I believe we need a different issue for documented non touched lines, making sure the source of the units since this change is an indication that is not checking it and assuming changing units to the required format and SI.
There was a problem hiding this comment.
Addressed in 57dd0d2: kept the utility inlet temperature on an absolute-temperature basis and clarified it inline as # [K], Utility inlet temperature. I also verified this against CoolingWater.temp_in defaulting to 298.15 and the jacket temperature state using K.
andres9403
left a comment
andres9403
left a comment
There was a problem hiding this comment.
Maintainer review (posted as COMMENT because this account (andres9403) authored the PR; GitHub rejects author APPROVE/REQUEST_CHANGES). Reviewing head 47c01c7, which has advanced past my prior review of 19f9ecb (the "Narrow unit annotations" commit).
Verdict
No blocking findings. The four issue #68 defects are each fixed correctly, the fixes match the established MSMPR/reactor Utility pattern, and the new tests genuinely lock in each fix (they are red against the pre-fix code shape). I would consider this merge-ready on the merits; a formal approval must come from another maintainer account since this account cannot approve its own PR (no branch-protection review gate is currently configured on master, so this is only the GitHub author restriction, not a reported required-review gate).
What I verified
- Defect 1 — MSMPR adiabatic branch (Crystallizers.py:1877-1891):
if self.adiabatic: ht_term = 0and the earlyreturn dtemp_dtwhenself.adiabatic or temp_ht is Nonecorrectly avoid thetemp_ht=Nonedereference and the spurious jacket tuple.test_msmpr_adiabatic_energy_balance_has_no_jacket_equationasserts a scalar return; pre-fix this hit theelif 'temp' in self.states_uopath and raisedTypeErrorontemp - None. - Defect 2 — Semibatch jacket inputs (Crystallizers.py:2218-2227): now reads
temp_in/vol_flowviaself.Utility.get_inputs(time)andcp/rhofromUtility. Confirmed against Utilities.py:16-17,67-71 thatget_inputsreturns those keys andcp/rhoexist. No lingering references to the removedself.temp_ht_in/self.flow_ht.test_semibatch_jacket_uses_utility_inputswas red pre-fix (AttributeError). - Defect 3 — liquid-feed enthalpy basis (Crystallizers.py:593-594):
h_in = h_in_mass * rho_liq_inconverts J/kg → J/m**3, consistent with the volumetric slurry enthalpyh_sp.test_liquid_feed_enthalpy_passed_to_energy_balance_is_volumetricasserts123*950; pre-fix returned the bare123(mass basis). - Defect 4 —
Slurry.getCpmutation (MixedPhases.py:331-341):volfracs = np.array(volfracs, copy=True)protects the caller's list before thetimes_vliqscaling, andself.epsilon = volfracs.copy()snapshots the pre-scaling fractions.self.epsilononSlurryhas no external readers, so the snapshot change is behavior-neutral outside this method.test_slurry_getcp_times_vliq_does_not_mutate_volfracswas red pre-fix.
Linked issue
Closes #68 is accurate: the PR addresses all four defects enumerated in the issue (not an adjacent subset), so the closing link is correct.
Tests run
pytest tests/test_crystallizer_energy_balances.py→ 4 passed (Assimulo stubbed in-test).pytest(default env) → 32 passed, 6 skipped (Assimulo unavailable; the skipped set is the integration tests, not the changed-behavior tests). Matches the PR body.- Warnings observed (
invalid escape sequence '\m'at Crystallizers.py:1057+) are pre-existing in untouched plotting code, not introduced here.
Merge-readiness
- PR-head checks:
Core testsSUCCESS,Assimulo integration testsSUCCESS on47c01c7. mergeStateStatus: CLEAN; file set matches (gh pr diff --name-only== local merge-base diff).- No blocking issues.
|
Addressed the remaining Bernalde units feedback on the Semibatch jacket lines and kept the Commits pushed:
Main changes:
Tests run:
GitHub checks on
Not expanded in this PR:
Remaining gate:
|
| rho_ht = 1000 | ||
| vol_ht = self.vol_tank*0.14 # m**3 | ||
| ht_media = self.Utility.get_inputs(time) | ||
| tht_in = ht_media['temp_in'] # [K] |
There was a problem hiding this comment.
This still seems to be an issue. Let’s please resolve it
| @@ -1884,6 +1887,9 @@ def energy_balances(self, time, params, cryst_rate, u_inputs, rhos, mu_n, | |||
| # Balance inside the tank | |||
| dtemp_dt = (flow_term - source_term - ht_term) / vol / capacitance | |||
There was a problem hiding this comment.
Does this comment refer to add units to not added line. If so I prefer not to add the units since as commented in the next commented line I believe the agent it is not checking the unit source but only using the SI units. I believe this requires a separate EPIC issue for documentation and to addressed model by model (Crystallizers, Dryer, etc)
There was a problem hiding this comment.
I disagree, every file we are touching should now incorporate units since in the process of fixing the bugs we should have the context to already fix this. Unless we encounter bugs, we can do this as a side cleanup. Adding units to any new/modified line is non-negotiable
There was a problem hiding this comment.
I dont thing it has already the context of fixing the units alongside while fixing the bugs for the entire function, and my fear is since we are adding units during this issues fixing we would believe its already sort it out. Thats why I needed to revert the changes in the first commit.
Thats why I propose a different issue (maybe not an epic) that only deals with documentation and unit adding.
There was a problem hiding this comment.
I think we have a confusion here. I agree with the adding units in the add/modified lines. Now since in the first commit I also ask to the agent that for any touched function the units should be added, when I realized that it changed the units of already commented ones (e.g changed "degC-> ["K"]) with no reason I reverted the changes, and even though the code line was not touched, due the change in the units you see this as a modified line.
There was a problem hiding this comment.
Addressed in 57dd0d2: dtemp_dt is now annotated as [K/s], and the adjacent MSMPR source term now records the volumetric cryst_rate [kg/m**3/s] basis.
| @@ -2210,11 +2216,12 @@ def energy_balances(self, time, params, cryst_rate, u_inputs, rhos, | |||
| # print(dtemp_dt) | |||
There was a problem hiding this comment.
There is some leftover comments that we should clean up
There was a problem hiding this comment.
Addressed in 57dd0d2: removed the leftover # print(dtemp_dt) debug comment from the Semibatch energy balance.
|
Addressed the remaining review feedback on current head Commits pushed:
Main changes:
Tests run:
No separate lint/type-check command is configured in GitHub checks on
Not intentionally declined:
Remaining gate:
|
| @@ -1545,7 +1546,7 @@ def energy_balances(self, time, params, cryst_rate, u_inputs, rhos, | |||
| height_liq = vol / (np.pi/4 * self.diam_tank**2) | |||
| area_ht = np.pi * self.diam_tank * height_liq + self.area_base # m**2 | |||
| dmaterial_dt = np.concatenate((ddistr_dt, dliq_dt)) | ||
|
|
||
| return dmaterial_dt, transf | ||
| return dmaterial_dt, transf # transf [kg/s] |
There was a problem hiding this comment.
What about the units of dmaterial_dt?
| h_in_mass = self.Inlet.getEnthalpy(temp=inlet_temp) # [J/kg] | ||
| h_in = h_in_mass * rho_liq_in # [J/kg]*[kg/m**3] -> [J/m**3] | ||
|
|
||
| phis_in = [1, 0] |
| dmaterial_dt = np.concatenate((ddistr_dt, dcomp_dt)) | ||
|
|
||
| return dmaterial_dt, transf | ||
| return dmaterial_dt, transf # transf [kg/m**3/s] |
There was a problem hiding this comment.
Why are the units different from the case above?
| return dmaterial_dt, transf | ||
| return dmaterial_dt, transf # transf [kg/m**3/s] | ||
|
|
||
| def energy_balances(self, time, params, cryst_rate, u_inputs, rhos, mu_n, |
There was a problem hiding this comment.
The function documentation would avoid the confusing units declaration below (where, in a single line cryst_rrate is also defined)
2 participants
Closes #68
Summary
Utility, matching the established MSMPR/reactor pattern.Slurry.getCpbeforetimes_vliqscaling so callers do not see mutatedvolfracs.Tests
pytest tests/test_crystallizer_energy_balances.pypytest(default environment: 32 passed, 6 skipped because Assimulo is unavailable there)/home/andresfel9403/anaconda3/bin/conda run -n PharmaPy python -m pytest(38 passed with Assimulo 3.4.3)git -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol diff --cached --checkBranch Hygiene
masterorigin/masterat4098d776613ecfab3d65e244a027cf58c3fe3c4afix/issue-68-crystallizer-energyupstream/masterat400e1e13ad4ecd3ba202d5fae8c70ec73bdf9c3eis already contained inorigin/master.