Modular Properties Inherent Reaction Scaling#1696
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## scaling_toolbox #1696 +/- ##
===================================================
+ Coverage 77.40% 77.43% +0.03%
===================================================
Files 395 395
Lines 64629 64691 +62
Branches 10859 10879 +20
===================================================
+ Hits 50026 50095 +69
+ Misses 12093 12088 -5
+ Partials 2510 2508 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| """ | ||
| # Autoscaling for constraints from a true species basis | ||
| # was not implemented in the legacy scaling tools. | ||
| # Use default scaling until we can rework the true to apparent |
|
|
||
|
|
||
| # TODO: Set a default state definition | ||
| # TODO: Need way to dynamically determine units of measurement.... |
There was a problem hiding this comment.
Can the UOM be determined from the constraint itself, or using flow basis and/or stoichiometry?
There was a problem hiding this comment.
I'm not sure what this TODO is about. At any rate, fixing it is outside the scope of this PR.
| """ | ||
| Method returning density of water | ||
| """ | ||
| return 55e3 |
There was a problem hiding this comment.
I added it, but I don't think it matters.
| """ | ||
| Dummy method for testing | ||
| """ | ||
| return 42 |
There was a problem hiding this comment.
Same question, since this is used for enthalpy below.
There was a problem hiding this comment.
I'd prefer not to. Frequently in test files like this, this method gets used for multiple properties with incompatible units, even if it is just used for enthalpy here.
| # "state_definition": None, | ||
| # "state_bounds": None, | ||
| # "state_components": None, |
There was a problem hiding this comment.
Are these commented-out lines here on purpose, or should we remove them?
There was a problem hiding this comment.
They're here on purpose, to make it clear these are fields that need to be set by the individual test to make that config work.
There was a problem hiding this comment.
I might just change "elsewhere" to "by the user". Right now, I think the wording is ambiguous as to whether it's being automatically specified somewhere else or if the user actually needs to specify themselves.
| # "state_definition": None, | ||
| # "state_bounds": None, | ||
| # "state_components": None, |
There was a problem hiding this comment.
Same comment as earlier on the "specified elsewhere"
| Config dictionaries used in testing of electrolyte states with and without | ||
| inherent reactions. | ||
|
|
||
| Authors: Andrew Lee, Douglas Allan |
There was a problem hiding this comment.
Since this file is 95% code copy-and-pasted from test files that Andrew wrote, I wanted to keep him as an author.
| # "state_definition": None, | ||
| # "state_bounds": None, | ||
| # "state_components": None, |
There was a problem hiding this comment.
I might just change "elsewhere" to "by the user". Right now, I think the wording is ambiguous as to whether it's being automatically specified somewhere else or if the user actually needs to specify themselves.
| # "state_definition": None, | ||
| # "state_bounds": None, | ||
| # "state_components": None, |
There was a problem hiding this comment.
Same comment as earlier on the "specified elsewhere"
| def dens_mol_h2o(*args, **kwargs): | ||
| return 55e3 |
There was a problem hiding this comment.
Is this even being called anywhere?
There was a problem hiding this comment.
It was an artifact from before I extracted the config dictionaries to a separate file.
MarcusHolly
left a comment
MarcusHolly
left a comment
There was a problem hiding this comment.
LGTM, although some of the other test files still have the dens_mol_H2O that needs to be deleted
dallan-keylogic
deleted the
modular_properties_inherent_reaction_scaler
branch
* combintorial explosion * finish scaler * extract electrolyte testing config * fire test * Pylint * unused import * no longer unreachable code * review comments * reviewer comments
3 participants
Summary/Motivation:
Scaling for electrolytes and inherent reactions for the modular property framework.
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: