-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add EPANET MSX enhancements #426
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could link directly to the "Building MSX models" section here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These links are broken. I assume we will want to update these to point to the corresponding files in the USEPA main branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbhart update links
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable -> variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbhart typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbhart remove this change.
wntr/tests/test_epanet_msx_io.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Pull in USEPA/wntr/main
add msx simulation test
…tion is not pulled in this commit
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/