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

Fix RE tests #185

Merged
merged 18 commits into from
Mar 28, 2023
Merged

Fix RE tests #185

merged 18 commits into from
Mar 28, 2023

Conversation

dguittet
Copy link
Contributor

Addresses issue:

removes test_h2_valve_opening since the valve is no longer being used in the RE models

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.md and COPYRIGHT.md file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

Copy link
Contributor

@radhakrishnatg radhakrishnatg left a comment

Choose a reason for hiding this comment

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

@dguittet Could you try adding TransformationFactory("network.expand_arcs").apply_to(m) before line 91 and see if this test passes?

@dguittet
Copy link
Contributor Author

@radhakrishnatg It won't pass when the Transformation is done. There are infeasible constraints. I'm not sure of the value of maintaining this Valve model test since it's no longer being used, and a working valve coefficient is finicky to identify, but I can try playing around with it to see if I can get some values that will solve.

@radhakrishnatg
Copy link
Contributor

@dguittet I think, TransformationFactory is definitely required for completeness. Otherwise, the required equality constraints which ensure that the outlet of the tank and the inlet of the valve the same will not be imposed.
But I agree with your point. It may not be worthwhile maintaining the test, since we are not using the valve model in any of our case studies.

radhakrishnatg
radhakrishnatg previously approved these changes Mar 27, 2023
bknueven
bknueven previously approved these changes Mar 27, 2023
@dguittet
Copy link
Contributor Author

@ksbeattie @lbianchi-lbl fixes tests that are failing in #177

@lbianchi-lbl
Copy link
Contributor

@dguittet Thanks for the fix. I've noticed that there's still one RE flowsheet test that's failing (from the looks of it, a solver error?):

image

I'm not sure if this is connected with the changes being introduced in this PR, of if it's a different, unrelated issue that happens to be triggered in the same test_RE_flowsheet.py test file.

@nareshsusarla
Copy link
Contributor

4 of the 5 failing tests are from the FE case study files and have been addressed in #179

@dguittet dguittet dismissed stale reviews from bknueven and radhakrishnatg via c6f2f52 March 28, 2023 15:03
@dguittet
Copy link
Contributor Author

dguittet commented Mar 28, 2023

Thanks @lbianchi-lbl and @nareshsusarla. I am looking into that SolverResults error in Github Actions, but can't reproduce it on my Mac or the HPC:

results = {'Problem': [{'Lower bound': -inf, 'Upper bound': inf, 'Number of objectives': 1, 'Number of constraints': 34847, 'Num...Phase Failed.', 'Termination condition': 'internalSolverError', 'Id': 501, 'Error rc': 0, 'Time': 128.07175731658936}]}

https://github.com/gmlc-dispatches/dispatches/actions/runs/4537252316/jobs/7994898724?pr=179

@lbianchi-lbl
Copy link
Contributor

lbianchi-lbl commented Mar 28, 2023

@dguittet thanks for looking into this. For the sake of getting out of the quasi-deadlock of test failures we're currently experiencing, I propose the following:

At that point, if you manage to fix the test_wind_battery[...] failure, we can then remove the xfail logic.

@dguittet
Copy link
Contributor Author

@lbianchi-lbl I restored this branch so that it's fixed at least one of the failing tests. To figure out the remaining one, I'm on another branch #188

@dguittet
Copy link
Contributor Author

@lbianchi-lbl I seem to be making progress on the remaining test. Perhaps I will get it fixed within the hour

@dguittet
Copy link
Contributor Author

@radhakrishnatg @bknueven @lbianchi-lbl I've resolved both the RE tests failures. Please re-review?

Copy link
Contributor

@radhakrishnatg radhakrishnatg left a comment

Choose a reason for hiding this comment

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

Looks good to me @dguittet

@lbianchi-lbl
Copy link
Contributor

@dguittet this is awesome, thanks! I'll go ahead and merge it right away. Once that's done, merging main into #179 (and possibly #181) should hopefully be enough to address all of the failures.

@lbianchi-lbl lbianchi-lbl self-requested a review March 28, 2023 20:11
@lbianchi-lbl lbianchi-lbl merged commit b73e289 into gmlc-dispatches:main Mar 28, 2023
@dguittet dguittet deleted the fix_test branch March 31, 2023 18:49
@ksbeattie ksbeattie added the Priority:Normal Medium Priority Issue or PR label Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Medium Priority Issue or PR
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

6 participants