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 Prescient as an optional dependecy, support tests with Prescient #467

Closed
adowling2 opened this issue Aug 18, 2021 · 12 comments · Fixed by #652
Closed

Add Prescient as an optional dependecy, support tests with Prescient #467

adowling2 opened this issue Aug 18, 2021 · 12 comments · Fixed by #652
Assignees
Labels
Priority:Normal Normal Priority Issue or PR

Comments

@adowling2
Copy link
Contributor

adowling2 commented Aug 18, 2021

The IDAES market integration feature #432 will include at least one regression test that involves Prescient. We should:

  • Add Prescient as an optional dependency
  • Add Prescient to the testing suite

@xiangao1 @ksbeattie @lbianchi-lbl

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Aug 19, 2021
@ksbeattie
Copy link
Member

@lbianchi-lbl, will work on the infrastructure (dependencies, testing) parts.
@dangunter, will look into using existing DMF example

@adowling2
Copy link
Contributor Author

@xiangao1 Can you add a link to the Prescient example notebook?

@xiangao1
Copy link
Contributor

@adowling2
Copy link
Contributor Author

adowling2 commented Jan 10, 2022

From @bknueven, working on closing one open Prescient PR, then cut a PyPi release

@adowling2
Copy link
Contributor Author

@lbianchi-lbl What are your thoughts on converting the above-linked notebook into a Prescient "existence" test? I think the test would essentially:

  • download the required data
  • run Prescient
  • check that the results files were created

@lbianchi-lbl
Copy link
Contributor

@adowling2 from what I can see from having skimmed through the notebook, I think this could be a reasonable option. A couple of questions:

  1. would be possible to "slim down" the current version to a minimal working version containing only the code that's strictly necessary for running the most basic workflow? This would help in terms of making the test simpler and more robust
  2. As I understand, the first step is to download some data, which I imagine involves making a network call. Is there any way that this test could be run without using the network, e.g. by bundling a stripped-down version of the data in the test directory? If this is not possible or too complicated, then that's fine, but for the purposes of the IDAES test suite, we should try to avoid tests that require making network calls since they tend to be more fragile/"flaky".

@xiangao1
Copy link
Contributor

Hi @lbianchi-lbl and @adowling2 , I learned that @bknueven has been working on this 5-bus example of Prescient, i.e., small scale, and it comes with the data: https://github.com/bknueven/Prescient/tree/5bus_case/examples

Maybe this is what we need.

@adowling2
Copy link
Contributor Author

Yes, this is actually what we want for an "existence" test.

@lbianchi-lbl
Copy link
Contributor

@xiangao1 Thanks for the pointer. At the link you posted, I can see some data files (simulate_5bus.txt and the 5bus subdirectory), however, the notebook itself (i.e. https://github.com/bknueven/Prescient/blob/5bus_case/examples/prescient_tutorial.ipynb) doesn't seem to reference either the file or the 5bus directory, and actually looks identical to the one you mentioned a few months ago in this issue (i.e. https://github.com/grid-parity-exchange/Prescient/blob/main/examples/prescient_tutorial.ipynb). Is there supposed to be a dedicated notebook somewhere for the 5bus example?

@bknueven
Copy link
Contributor

Ignore the notebook -- it needs to be updated. I'll add a Python file to the PR I'm already working on with that test case.

@bknueven
Copy link
Contributor

Here is a good example runner file: https://github.com/bknueven/Prescient/blob/5bus_case/examples/simulate_5bus.py

That takes about five minutes on my system. For the purposes of this test, we can change a few options to lower that time to 15 seconds.

@lbianchi-lbl
Copy link
Contributor

@bknueven that's awesome, thanks! I'll open a draft PR in IDAES using that code and data files. I understand that it might not work immediately, but at least we'll have a starting point to work on.

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 a pull request may close this issue.

6 participants