JOSS Review

[kbonney]

Hi @andreArtelt, nice work on the package and paper. Below are some comments for improving both. The only points that I need resolved to complete my review are marked by a ❗, the rest are suggestions you can take or leave now or later.

Functionality

• The software relies on software that isn't actively maintained umsgpack. I don't think this is something that needs to be directly addressed, but I do want to point out that relying on unmaintained packages can cause issues down the line as the package could possibly break on new releases of Python.

Documentation

• ❗ I haven't found the statement of need anywhere in the docs. I've seen people put this in the repo's README or the landing page of their documentation website.
• ❗ When I run the test suite all tests pass except for the benchmark tests which seem to get caught up on some request issues. I'll work on this a little bit more but if I can't resolve it I'll open a separate issue about the problem.
• My understanding is that the primary use case of EPyT-Flow is to integrate with machine learning, so I think it would be beneficial to include an example of what this might look like. Similarly, those unfamiliar with a REST API would benefit from an example of what they could do with the one you've provided. This could be a concrete example or just an explanation of REST with some suggested use cases.
• ❗ Contributions guidelines are very minimal. I suggest adding templates for Issues and PRs (you can find plenty of examples online) and also adding some text somewhere in your README or docs addressing how to seek support.

Software Paper

• ❗ In the section "State of the Field" there is a statement that WNTR does not support quality dynamics, however basic quality functionality is currently available (see https://usepa.github.io/WNTR/waterquality.html) and advanced quality analysis via the MSX module is nearing completion (see Add EPANET MSX enhancements USEPA/WNTR#426). Similarly, there are controls available in WNTR (see https://usepa.github.io/WNTR/controls.html). I recommend dropping the mention of quality and controls from the limitations of WNTR. From what I can see, the simulation capabilities of WNTR and EPyT-Flow are pretty similar, so I would focus on the functionality of EPyT-Flow that facilitates systematic generation and simulation of scenarios such as the integration of uncertainty and the Gym environment to differentiate the package from WNTR.
• The software mentions seven benchmarks which can be used for evaluating algorithms (line 71-73), but it isn't entirely clear what this means. It would be beneficial to give readers some sort of vision for how you imagine people using these benchmarks. The discussion of uncertainty could also be expanded. I have an AI background so it is clear to me how the uncertainty module would be useful for building datasets, for example, but that might not be obvious to someone without that background.

[andreArtelt]

Thanks for your review -- much appreciated 🙏.
I was busy with administrative tasks the last days after I returned from a longer vacation -- sorry about the delay.

[andreArtelt]

Your comment on controls is very similar to the one raised by the other reviewer in #9
I think I did not make this clear enough in the paper. EPyT-Flow supports arbitrary controls that go beyond "simple" IF-THEN-ELSE rules as supported by EPANET. For instance, you could use a neural network to make control decisions based on the observed sensor readings. I added a clarifying statement to the paper and documentation in commit dee0de5
Let me know, what you think.

[andreArtelt]

Regarding your comment on missing quality dynamics in WNTR:
You are right that WNTR provides access to EPANET quality dynamics and hopefully to EPANET-MSX as well soon. However, if I understood it correctly, those simulators can not be used with the other (custom) simulator and therefore lack some features such as the effect of leakages, earth quakes, etc.. This is not the case in EPyT-Flow, here everything is integrated into a single simulator -- e.g. leakages and quality dynamics can be modeled together.

My suggestion would be to relax and rephrase the statement in the paper, e.g. "does not fully integrate (advanced) quality dynamics" or smth. similar. What do you think?

[andreArtelt]

Right now, the statement of need is only in the paper. However, in efa61b5 I just added it to the landing page of the documentation as well.

[andreArtelt]

Regarding your comment on contribution guidelines: I did not know about those templates -- thanks a lot for pointing this out to me. I just added issue and pull-request templates and also added a short comment to the README on how to get support.

Let me know if you suggest anything else.

[kbonney]

Your comment on controls is very similar to the one raised by the other reviewer in #9 I think I did not make this clear enough in the paper. EPyT-Flow supports arbitrary controls that go beyond "simple" IF-THEN-ELSE rules as supported by EPANET. For instance, you could use a neural network to make control decisions based on the observed sensor readings. I added a clarifying statement to the paper and documentation in commit dee0de5 Let me know, what you think.

Thanks, that helps clarify the differences. It is also nice to highlight the use of ML throughout the paper, as I think that aspect is a bit understated.

Regarding your comment on missing quality dynamics in WNTR: You are right that WNTR provides access to EPANET quality dynamics and hopefully to EPANET-MSX as well soon. However, if I understood it correctly, those simulators can not be used with the other (custom) simulator and therefore lack some features such as the effect of leakages, earth quakes, etc.. This is not the case in EPyT-Flow, here everything is integrated into a single simulator -- e.g. leakages and quality dynamics can be modeled together.

My suggestion would be to relax and rephrase the statement in the paper, e.g. "does not fully integrate (advanced) quality dynamics" or smth. similar. What do you think?

Yes, the leaks in the custom simulator can't be used with the EPANET simulator. I think in your rephrasing I would mention the specific cases you brought up. "does not fully integrate (advanced) quality dynamics with scenarios such as pipe leaks."

[kbonney]

Right now, the statement of need is only in the paper. However, in efa61b5 I just added it to the landing page of the documentation as well.

Thanks. That placement looks good, but I haven't seen it update on the actual documentation page. Not sure what triggers the docs to be updated, perhaps it is only on releases?

[kbonney]

Regarding your comment on contribution guidelines: I did not know about those templates -- thanks a lot for pointing this out to me. I just added issue and pull-request templates and also added a short comment to the README on how to get support.

Let me know if you suggest anything else.

These look great, thanks. I'll mark the item as resolved.

[andreArtelt]

Right now, the statement of need is only in the paper. However, in efa61b5 I just added it to the landing page of the documentation as well.

Thanks. That placement looks good, but I haven't seen it update on the actual documentation page. Not sure what triggers the docs to be updated, perhaps it is only on releases?

Yes, the documentation is only rebuilt in case of a new release. However, you can check out the documentation of the dev branch on https://epyt-flow.readthedocs.io/en/latest/ -- I just triggered the build of this one manually, so that you can see how it will look like if I make a new release.

[andreArtelt]

Your comment on controls is very similar to the one raised by the other reviewer in #9 I think I did not make this clear enough in the paper. EPyT-Flow supports arbitrary controls that go beyond "simple" IF-THEN-ELSE rules as supported by EPANET. For instance, you could use a neural network to make control decisions based on the observed sensor readings. I added a clarifying statement to the paper and documentation in commit dee0de5 Let me know, what you think.

Thanks, that helps clarify the differences. It is also nice to highlight the use of ML throughout the paper, as I think that aspect is a bit understated.

Regarding your comment on missing quality dynamics in WNTR: You are right that WNTR provides access to EPANET quality dynamics and hopefully to EPANET-MSX as well soon. However, if I understood it correctly, those simulators can not be used with the other (custom) simulator and therefore lack some features such as the effect of leakages, earth quakes, etc.. This is not the case in EPyT-Flow, here everything is integrated into a single simulator -- e.g. leakages and quality dynamics can be modeled together.
My suggestion would be to relax and rephrase the statement in the paper, e.g. "does not fully integrate (advanced) quality dynamics" or smth. similar. What do you think?

Yes, the leaks in the custom simulator can't be used with the EPANET simulator. I think in your rephrasing I would mention the specific cases you brought up. "does not fully integrate (advanced) quality dynamics with scenarios such as pipe leaks."

Done in 21b9344 -- I also added a few sentenced highlighting the benefits for AI and ML research

[andreArtelt]

When I run the test suite all tests pass except for the benchmark tests which seem to get caught up on some request issues. I'll work on this a little bit more but if I can't resolve it I'll open a separate issue about the problem.

Does this issue still exist? All tests run successfully on my machine as well as on GitHub (see Actions) -- all tests are run when a commit is pushed.