Add EPANET MSX enhancements #426
Conversation
The class will be similar to the WaterNetworkModel in terms of its structure and options.
Merge main into development branch
Citations
Update to add basic citation class
Exceptions
* Update to exception documentation * Update documentation for EN errors
* Updated MSX binaries, PR issued to EPANETMSX main * Updated toolkit to match modified code in MSX DLL * Fixed an error in the filename encoding
* Move library to new directory * Updates to documentation * New ipynb demo * various bug fixes
There was a problem hiding this comment.
Overall, PR is well organized and well written. The MSX code and API follows the current WNTR style and is integrated clearly with the existing code base. Documentation and examples are provided to help users learn to use the new MSX features. A test suite is provided, but should be expanded to cover more of the code and common use cases.
I have commented on some minor items throughout. Aside from a typo preventing correct code execution in the example, nothing is of high importance. I recommend that we expand the test suite before merging, which I'd be happy to help with.
documentation/conf.py
Outdated
There was a problem hiding this comment.
What are these options changing?
|
|
||
| The EpanetSimulator can use EPANET-MSX 2.0 :cite:p:`shang2023` to run | ||
| multi-species water quality simulations. | ||
| Additional multi-species simulation options are discussed in :ref:`advanced_simulation`. |
There was a problem hiding this comment.
Could link directly to the "Building MSX models" section here.
| :class:`~wntr.msx.library.ReactionLibrary`. | ||
| This includes the following models: | ||
|
|
||
| * `Arsenic oxidation/adsorption <https://github.com/dbhart/WNTR/blob/msx/wntr/msx/_library_data/arsenic_chloramine.json>`_ :cite:p:`shang2023` |
There was a problem hiding this comment.
These links are broken. I assume we will want to update these to point to the corresponding files in the USEPA main branch.
| and **parameters**. Constants have a single value in every expression. Parameters have a global value | ||
| that is used by default, but which can be modified on a per-pipe or per-tank basis. | ||
|
|
||
| Pre-defined hydraulic variable can be found in |
There was a problem hiding this comment.
- Suggest adding a direct link to epa msx user manual.
- Suggest making T/T1 consistent (choose one or the other instead of having both throughout) .
- In Cell 11, recommend removal of leftover testing comment.
- Suggest adding axes labels for last figure from cell 9 .
- Cell 17 naming error, need to lowercase mutlisource-cl.
| If `variable_or_name` is not a term in the model | ||
| """ | ||
| name = str(variable_or_name) | ||
| # FIXME: validate deletion |
| raise SyntaxError("Unknown first word in [QUALITY] section") | ||
| if spec not in self.rxn.reaction_system.species: | ||
| raise ValueError("Undefined species in [QUALITY] section: {}".format(spec)) | ||
| # FIXME: check for existence |
| raise ValueError("%s is not a valid member of MsxSolverOptions") | ||
| self.__dict__[name] = value | ||
|
|
||
| def to_dict(self): |
There was a problem hiding this comment.
Suggest providing this for MsxReportOptions as well. Alternatively, could be added to _OptionsBase since it is generic enough and would be useful for all subclasses.
There was a problem hiding this comment.
@dbhart bump up to OptionsBase
wntr/network/base.py
Outdated
| @@ -575,8 +589,8 @@ class Registry(MutableMapping): | |||
| """ | |||
|
|
|||
| def __init__(self, wn): | |||
| if not isinstance(wn, AbstractModel): | |||
| raise ValueError('Registry must be initialized with a model') | |||
| # if not isinstance(wn, AbstractModel): | |||
There was a problem hiding this comment.
Is this removed to allow use with MSX classes? If so, I suggest keeping and extending the allowed instance types to cover MSX.
There was a problem hiding this comment.
@dbhart remove this change.
wntr/tests/test_epanet_msx_io.py
Outdated
There was a problem hiding this comment.
Suggest adding additional checks for options, pipes, tanks, sources, and initial conditions.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class MsxLibrary: |
There was a problem hiding this comment.
It might be worth abstracting part of the MsxLibrary interface to allow for extensions to librarys for other purposes (eg demand pattern library). Not sure how this might look but I'd be interested in discussing it further.
documentation/conf.py
Outdated
There was a problem hiding this comment.
@dbhart TODO remove these settings
| raise TypeError('Expected QualityModelBase (or derived), got {}'.format(type(model))) | ||
| self._msx = model | ||
|
|
||
| def add_msx_model(self, msx_filename=None): |
There was a problem hiding this comment.
Suggest changing keyword arg from msx_filename to msx_file_name to follow WNTR convention (eg wn model constructor uses inp_file_name). @dbhart
This provides updates to WNTR that integrate EPANET-MSX into the EpanetSimulator.
In addition to the new simulation options, this provides MSX model objects, such as species, terms, etc., that can be added to a water network model in a similar way to how WNTR works currently.
Demonstration is located in examples/demos/