Skip to content
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

Pulling in community updates #6

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Pulling in community updates #6

wants to merge 9 commits into from

Conversation

pshassett
Copy link
Owner

Thanks for keeping this alive and up-to-date with the progress of WNTR.

Copy link
Owner Author

@pshassett pshassett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections to most of these changes.

@pshassett
Copy link
Owner Author

@jhogge
@jckettepa

I see that the tests haven't been passing.
I propose making a new branch here to pull in the updates and we can collaboratively get the tests to pass.
Thoughts?

@jhogge
Copy link
Contributor

jhogge commented Apr 8, 2021

@pshassett
We are generally seeing differences in the third significant figure on the tests ever since the WNTR 0.3.0 update, so I'm thinking there was a change to the WNTRSimulator that slightly modified the testing results. I think @jckettepa has looked into WNTR vs. EPANET simulator results?

@pshassett
Copy link
Owner Author

@jhogge thanks for the quick response. I suppose the benchmark values just need to be updated then.
I am seeing some package version issues coming up on my tests for this particular PR.

@jckett
Copy link

jckett commented Apr 8, 2021

Hi all, yes we are seeing minor changes between WNTR and EPANET simulator. Additionally, with the change of WNTR incorporating EPANET 2.2, we have gotten slightly different pressure results for the criticality runs since @pshassett ran them. The recent PR only changes the segment indexing so I'm not sure why this specific PR would show errors for you but the November one did not. I re-cloned @jhogge's repo today and pushed the index change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants