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

Add optional requirement and simple test case for Prescient #652

Merged
merged 18 commits into from
Apr 5, 2022

Conversation

lbianchi-lbl
Copy link
Contributor

@lbianchi-lbl lbianchi-lbl commented Jan 11, 2022

Fixes: #467

Summary/Motivation:

Changes proposed in this PR:

  • Add gridx-prescient as an optional dependency in setup.py
  • Add simple test case with built-in input CSV files (so that no network calls are required to run the tests)

TODO before undrafting

Reviewers

  • @bknueven (for some reason I wasn't able to add you through the usual menu when creating the 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.txt 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

@bknueven bknueven left a comment

Choose a reason for hiding this comment

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

The suggested option changes below should get you a much faster test.

idaes/tests/prescient/test_prescient.py Outdated Show resolved Hide resolved
idaes/tests/prescient/test_prescient.py Outdated Show resolved Hide resolved
idaes/tests/prescient/test_prescient.py Outdated Show resolved Hide resolved
@lbianchi-lbl
Copy link
Contributor Author

lbianchi-lbl commented Jan 11, 2022

@bknueven thanks for the updated options! The runtime is ~10-15 s on the GHA runners, which I think is just fine for a component test.

Do you have any suggestion for a basic validation of the results once the simulation is complete? The current implementation just asserts that at least two files have been created in the output directory, which is not a particularly stringent check...

@bknueven
Copy link
Contributor

Prescient's own regression tests are available here: https://github.com/grid-parity-exchange/Prescient/blob/main/tests/simulator_tests/test_sim_rts_mod.py

That said, something simpler might just be to check for the existence of overall_simulation_output.csv in the solution directory, which is only created and written to as one of the final steps of Prescient execution.

If you want to check the values in any of the files, you should update the ruc_mipgap option to 0.0. Also, it's probably a bad idea to use strict file equality as we occasionally add columns to these csv files if we find something else worth reporting.

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #652 (559b788) into main (8310071) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #652   +/-   ##
=======================================
  Coverage   67.23%   67.23%           
=======================================
  Files         528      528           
  Lines       60358    60358           
  Branches    11095    11095           
=======================================
+ Hits        40579    40582    +3     
- Misses      17692    17693    +1     
+ Partials     2087     2083    -4     
Impacted Files Coverage Δ
idaes/ver.py 61.53% <0.00%> (-4.62%) ⬇️
idaes/core/util/env_info.py 79.36% <0.00%> (-3.18%) ⬇️
idaes/dmf/model_data.py 52.04% <0.00%> (+0.51%) ⬆️
idaes/dmf/util.py 54.04% <0.00%> (+1.27%) ⬆️
idaes/dmf/codesearch.py 100.00% <0.00%> (+1.44%) ⬆️
idaes/conftest.py 90.90% <0.00%> (+6.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8310071...559b788. Read the comment docs.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Jan 13, 2022
@lbianchi-lbl
Copy link
Contributor Author

@bknueven Is there a planned timeline for when a new version of Prescient including these changes will be released on PyPI? From the IDAES side, I think this is ready to go, but I imagine it would be preferable to use a PyPI release (rather than main) to specify the requirement.

@bknueven
Copy link
Contributor

We could cut the Prescient release when convenient. We were hoping to get one more feature in, but we also don't want to hold up the IDAES release or have IDAES released against a specific Prescient commit.

@lbianchi-lbl
Copy link
Contributor Author

@bknueven Thanks for the update. My question for purely for keeping track of this PR's status: personally I don't know how urgent it would be to have this merged into IDAES (and therefore how urgently we'd need to have the Prescient PyPI release).

@adowling2, do you have any thoughts on the required or desired timeline for enabling Prescient in IDAES?

@lbianchi-lbl lbianchi-lbl marked this pull request as ready for review March 1, 2022 16:50
@bknueven
Copy link
Contributor

bknueven commented Mar 1, 2022

Does not matter for this PR, but grid-parity-exchange/Prescient#139 distributes the Prescient regression test harness for doing regression tests of Prescient output. Perhaps useful for #638.

bknueven
bknueven previously approved these changes Mar 1, 2022
Copy link
Contributor

@bknueven bknueven left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me

setup.py Outdated Show resolved Hide resolved
@bknueven
Copy link
Contributor

bknueven commented Apr 1, 2022

This should be merged before #638 so #638 can include Prescient integration tests.

ksbeattie
ksbeattie previously approved these changes Apr 2, 2022
Copy link
Member

@ksbeattie ksbeattie left a comment

Choose a reason for hiding this comment

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

LGTM

.github/workflows/core.yml Outdated Show resolved Hide resolved
@lbianchi-lbl lbianchi-lbl enabled auto-merge (squash) April 5, 2022 22:26
Copy link
Member

@ksbeattie ksbeattie left a comment

Choose a reason for hiding this comment

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

LGTM

@lbianchi-lbl lbianchi-lbl merged commit 70784c7 into IDAES:main Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Prescient as an optional dependecy, support tests with Prescient
3 participants