Update _components.py

[BenGillen1998]

As mentioned in issue 100, I added a flag to the compile method. Now, components which have a measured_as property will not compile without switching the flag.

[codecov]

Codecov Report

Attention: Patch coverage is 96.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.76%. Comparing base (e45b7b4) to head (55c6704).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
qsdsan/_components.py	90.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
+ Coverage   72.75%   72.76%   +0.01%     
==========================================
  Files          98       98              
  Lines       19645    19653       +8     
  Branches     1702     1706       +4     
==========================================
+ Hits        14292    14301       +9     
+ Misses       4789     4788       -1     
  Partials      564      564              

Flag	Coverage Δ
unittests	72.76% <96.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yalinli2]

@BenGillen1998 thanks for pushing the changes! I fixed the tests throughout. exposan tests are failing since these haven't been merged; all the tests passed locally.

@joyxyz1994 I tried to add some docs related to this for default_compile, but unsure how MW should be properly set, can you add some explanation in the doc, preferably with some examples to show correct vs. wrong way of setting MW for components with measured_as attributes? Thanks!

[joyxyz1994]

@BenGillen1998 thanks again for leading this effort! And thanks @yalinli2 for reconciling the tests and docstrings!

On top of the changes you've made, I incorporated another kwarg adjust_MW_to_measured_as to the default_compile method. If set to True by the user, when measured_as is not None and every component has a formula, this automatically adjusts the MW values of the components during compile, which addresses the inaccuracy and avoids error even if ignore_inaccurate_molar_weight is False. Documentation and examples are added accordingly.

[joyxyz1994]

@yalinli2 while addressing this issue, I found the inaccurate molecular weights could have significant impacts on calculation results of some units/streams. An example is the biogenic refinery units, which involve concentrated streams (e.g., biosolids, concentrated nutrient stream) and the _run functions often used <WasteStream>.F_vol and mass concentration-based properties (e.g., <WasteStream>.TN) rather than mass flows directly. Because MWs were typically defaulted to 1, F_vol tends to be inflated, especially for concentrated streams. We should consider updating those _run functions. Otherwise, setting the correct MWs will likely change the model results for those systems.

[joyxyz1994]

@yalinli2 An update regarding the issue above. The error in F_vol caused by inconsistent MW and measured_as can typically be cancelled out by the error in <WasteStream>.TN. So, estimating mass flow from F_vol and TN isn't an issue for liquid stream despite the MW inconsistency. However, if the MW is inconsistent with measured_as, <Stream>.imol simply returns a wrong value -- this is the main cause for the failed tests for "biogenic_refinery" and "reclaimer", since N mass flow in the recovered conc_NH3 stream was estimated using imol['NH3'] whereas NH3 is measured as N but has MW=17.

An attempt to fix the wrong mass flow calculations is included in the recent commit to EXPOsan. Unfortunately, this means we might have to rerun all simulations if N recovery % is important.

[yalinli2]

@joyxyz1994 thanks for looking into this!

To make sure we understand this correctly, before any of these changes, NH3's measured_as was set to N. Say we have 14 mg/L NH3 measured as N, N should be 1 mmol. However the formula was given as NH3, so the MW was auto-calculated as 17 g/mole, N was calculated to 14/17 = 0.82 mmol.

When calculating the recovery, the unit script used a fixed 14, so it became 14*0.82 = 11.5 mg/L, even we recover all N.

However, if TN was used, because it considered the discrepancy through i_mass, which would be 17/14 = 1.2, so it will give the correct result of 0.82*1.2 = 1 mmol N.

In some of the EXPOsan modules, TN, TP, TK were used in calculating recoveries, and the results were therefore correct. But this was not the case for biogenic_recovery and reclaimer, so the results were wrong.

Was my interpretation correct?

[joyxyz1994]

@yalinli2 That's correct! The reason why there's issue with biogenic_refinery and reclaimer is because some of the "recovered" streams are non-liquid streams, i.e., they don't have TN, TP, TK properties. So in the metric getter definitions, imol was used instead.

[yalinli2]

@joyxyz1994 oh got it.

I just checked some other modules, new_generator seems to be doing the right way, scg_zyclonic and eco_san don't seem to have such calculations.

ah, do you know how different the recoveries would be? This is probably just for NH3, not even for NonNH3 because it didn't have a formula.

correction: you are right, this line would cause problem: https://github.com/QSD-Group/EXPOsan-private/blob/main/exposan/new_generator/__init__.py#L328

[joyxyz1994]

@yalinli2 , for both biogenic refinery system B and reclaimer systems B&C, it underestimated the baseline N recovery % by about 10% (absolute). And you're right, I only found it to be an issue for NH3 not other forms of N or other nutrients.

[joyxyz1994]

Algorithms for estimating N recovery % have been updated for biogenic_refinery, reclaimer, and new_generator. Tests in the public EXPOsan repo have updated accordingly in commit.