gmlc-dispatches / dispatches

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

Use CBC in NE double loop notebook #202

Closed radhakrishnatg closed 1 year ago

radhakrishnatg commented 1 year ago

Addresses issue:

Fixes notebook error in #200

Summary/Motivation:

Changes proposed in this PR:

-

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

Hi @lbianchi-lbl The RE tests that were failing in #199 are also failing in this PR. And, from what I understand, this PR does not include the changes in IDAES 1180. So does that mean that the test failures in #199 are not related to the changes in IDAES 1180?

lbianchi-lbl commented 1 year ago

Hi @lbianchi-lbl The RE tests that were failing in #199 are also failing in this PR. And, from what I understand, this PR does not include the changes in IDAES 1180. So does that mean that the test failures in #199 are not related to the changes in IDAES 1180?

Most likely, that's correct. #199 updates the IDAES requirement, which this PR does not. However, Pyomo 6.6.0 was released 2 days ago, and some of the errors we're seeing now seem to be directly related to that. I've created an issue for the UnboundLocalError (Pyomo/pyomo#2846), but I'm not sure about the solver failures.

In the meantime, while we're still able to do so (i.e. before switching to a version of IDAES that depends on pyomo>6.5.0) we can add a pyomo<6.6 requirement to setup.py. I can push directly to this branch if that's OK.

radhakrishnatg commented 1 year ago

Sure, @lbianchi-lbl I'm okay with that. That way, we would know if it is a pyomo issue or DISPATCHES issue. Also, I mentioned a comment in another PR. I'm not seeing the RE test failures locally.

dguittet commented 1 year ago

On my mac or on the HPC, I'm also not able to reproduce the IPOPT failures in these tests:

FAILED dispatches/case_studies/renewables_case/tests/test_RE_flowsheet.py::test_wind_battery_pem_tank_turb_optimize_simple - ValueError: Cannot load a SolverResults object with bad status: error
FAILED dispatches/case_studies/renewables_case/tests/test_RE_flowsheet.py::test_wind_battery_pem_tank_turb_optimize_detailed - assert 34.74003128616143 == 0 ± 3.0e+00

Edit: this is updating Pyomo to 6.6.0 and using the same version of IPOPT (3.13.2)

codecov[bot] commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (03b936a) 89.49% compared to head (08d358c) 89.49%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #202 +/- ## ======================================= Coverage 89.49% 89.49% ======================================= Files 67 67 Lines 8238 8238 ======================================= Hits 7373 7373 Misses 865 865 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

lbianchi-lbl commented 1 year ago

@radhakrishnatg will try running again after updating to the current dependencies (IDAES 2.0/Pyomo 6.5) (ensure pip show idaes-pse pyomo to confirm the versions installed) and see if the error persists.

lbianchi-lbl commented 1 year ago
lbianchi-lbl commented 1 year ago

We can merge this while investigating the issue reported by @radhakrishnatg since it's likely not related (and the cell is skipped in CI anyway).