ASM2d/ADM1 Translator Update #1315
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1315 +/- ##
==========================================
+ Coverage 94.41% 94.42% +0.01%
==========================================
Files 370 370
Lines 37753 37740 -13
==========================================
- Hits 35645 35637 -8
+ Misses 2108 2103 -5 ☔ View full report in Codecov by Sentry. |
| @@ -150,8 +150,8 @@ Equations and Relationships (With Decay) | |||
| "S_K concentration", ":math:`S_{K, 6} = S_{K, in} + (K_{XPP} * X_{PP, in})`" | |||
| "S_Mg concentration", ":math:`S_{Mg, 6} = S_{Mg, in} + (Mg_{XPP} * X_{PP, in})`" | |||
|
|
|||
| Equations and Relationships (Without Decay) | |||
| ------------------------------------------- | |||
| Equations and Relationships (with phosphorus biomass handled implicitly) | |||
There was a problem hiding this comment.
One of these should be labeled without biological phosphorus removal and the other with
| class BioP(Enum): | ||
| on = auto() | ||
| off = auto() | ||
|
|
||
|
|
There was a problem hiding this comment.
| class BioP(Enum): | |
| on = auto() | |
| off = auto() |
There was a problem hiding this comment.
I think we can handle this without an Enum class (same for decay switch)
| default=BioP.on, | ||
| domain=In(BioP), |
There was a problem hiding this comment.
| default=BioP.on, | |
| domain=In(BioP), | |
| default=True, | |
| domain=Bool, |
| "``BioP.on``", "All the BioP variables are supposed to be transformed in the interface" | ||
| "``BioP.off``", "All the BioP variables are kinetically described within the ADM" |
There was a problem hiding this comment.
Change to True/False instead of Enum options
|
|
||
| # -------------------------------------------Step 4----------------------------------------------------------------# | ||
| if self.config.inlet_reaction_package.config.decay_switch == DecaySwitch.on: | ||
| if self.config.bio_P == BioP.on: |
There was a problem hiding this comment.
| if self.config.bio_P == BioP.on: | |
| if self.config.bio_P: |
|
|
||
| # -----------------------------------------Step 4--------------------------------------------------------------# | ||
| elif self.config.inlet_reaction_package.config.decay_switch == DecaySwitch.off: | ||
| elif self.config.bio_P == BioP.off: |
There was a problem hiding this comment.
| elif self.config.bio_P == BioP.off: | |
| elif not self.config.bio_P: |
|
|
||
| # -----------------------------------------Step 4--------------------------------------------------------------# | ||
| elif self.config.inlet_reaction_package.config.decay_switch == DecaySwitch.off: | ||
| elif self.config.bio_P == BioP.off: |
There was a problem hiding this comment.
I couldn't see it with all the truncating that Github does, but it could be good to have an else here or following this. If the logic really only comes down to Bio_P being True or False, then this could just be else.
There was a problem hiding this comment.
I don't have an else - I'll add it here
| @@ -111,8 +111,8 @@ Equations and Relationships | |||
| "S_PO4 concentration", ":math:`S_{PO4, 3} = S_{PO4, 2} + (S_{F, in} * i_{PSF})`" | |||
| "S_IC concentration", ":math:`S_{IC, 3} = S_{IC, 2} + (S_{F, in} * i_{CSF}) - (S_{su} * Ci[S_{su}] * 1000 * 12) - (S_{aa} * Ci[S_{aa}] * 1000 * 12)`" | |||
|
|
|||
| Equations and Relationships (With Decay) | |||
| ---------------------------------------- | |||
| Equations and Relationships (with biological phosphorus removal) | |||
There was a problem hiding this comment.
So is Decay omitted for the time being or is it somewhere else in this file and associated code?
There was a problem hiding this comment.
The use of the word decay was inaccurate to begin with. That is entirely handled in the reaction package (depending on whether the DecaySwitch is on(True) or off (False). Not sure if that answers your question...
There was a problem hiding this comment.
Got it- the decay switch isn't applied in the translator but is applied in the reaction package. Thanks!
| default=DecaySwitch.on, | ||
| domain=In(DecaySwitch), | ||
| default=True, | ||
| domain=Bool, | ||
| description="Switching function for decay", | ||
| doc=""" | ||
| Switching function for handling decay in reaction rate expressions. | ||
|
|
||
| **default** - `DecaySwitch.on`` | ||
|
|
||
| .. csv-table:: | ||
| :header: "Configuration Options", "Description" | ||
|
|
||
| "``DecaySwitch.on``", "The decay of heterotrophs and autotrophs is dependent on the electron acceptor present" | ||
| "``DecaySwitch.off``", "The decay of heterotrophs and autotrophs does not change" | ||
| """, | ||
| doc="""Switching function for handling decay in reaction rate expressions, | ||
| **default** - True. | ||
| **Valid values:** { | ||
| **True** - the decay of heterotrophs and autotrophs is dependent on the electron acceptor present, | ||
| **False** - the decay of heterotrophs and autotrophs does not change""", |
There was a problem hiding this comment.
@MarcusHolly Does this need to be updated on the technical ref ?
There was a problem hiding this comment.
It is updated automatically
There was a problem hiding this comment.
I suspected this but didn't check if any manually written description conflicted with this.
There was a problem hiding this comment.
The manual descriptions of decay are all still accurate I believe.
Summary/Motivation:
This PR decouples the translator block configuration argument (previously DecaySwitch), and introduces a separate configuration option under the name BioP.
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: