gmlc-dispatches / dispatches

Primary repository for distributed dispatches software tools
https://dispatches.readthedocs.io/
Other
12 stars 34 forks source link

Fix RE tests #185

Closed dguittet closed 1 year ago

dguittet commented 1 year ago

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.
dguittet commented 1 year ago

@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 commented 1 year ago

@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.

dguittet commented 1 year ago

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

lbianchi-lbl commented 1 year ago

@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 commented 1 year ago

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

dguittet commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

dguittet commented 1 year ago

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

lbianchi-lbl commented 1 year ago

@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.