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

Price taker model for DISPATCHES, Rehashed #1358

Open
wants to merge 129 commits into
base: main
Choose a base branch
from

Conversation

djlaky
Copy link

@djlaky djlaky commented Feb 29, 2024

Fixes

Compared to #1201, operational constraints mathematical form was corrected. Unnecessary functions were removed/merged. Additional user flexibility was added for constructing cost objectives.

Summary/Motivation:

Resurrecting #1201 to finish price taker framework in accordance with project milestones.

Framework allows the user to construct price-taker models for design and/or operational optimization considering time-varying market price data.

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.

radhakrishnatg and others added 30 commits May 25, 2023 21:46
… functions to add constraints through pyomo blocks
@ksbeattie
Copy link
Member

@MarcusHolly @djlaky, we need an update on this.

@MarcusHolly
Copy link
Contributor

We're going back to working on this. Hopefully it can be merged this month

@ksbeattie
Copy link
Member

@djlaky @MarcusHolly what's the news on this?

@MarcusHolly
Copy link
Contributor

It's being looked at (and hopefully finished) for this month

@MarcusHolly
Copy link
Contributor

  1. We will need documentation for all the new classes - at a minimum some technical references for the API (autodocs), but and explanation with a simple demonstration of how to use the tools would be nice if possible (looking at the code, this will be very important).
  2. I do not see a test file for design_and_operation_model.py. This might be rolled into another test, but I recommend having a separate test file for this to help with future maintenance.
  3. In general, I think you could (and should) do a lot more unit testing of methods in isolation. From what I can see in the tests, there are not a lot of tests that target individual methods. E.g. I see no test that targets add_startup_shutdown and actually tests that it does what it is supposed to do (I see one test for a failure case, but that is it).
  4. Most of the tests I see are based on what I assume is an actual case study. I would suggest thinking about whether you can construct simpler dummy cases for unit testing all the methods in isolation, and where possible mock up any necessary starting information so that you have fewer moving parts and simpler cases to test with. The full case study is good for an end-to-end component/integration test of the system, but often make it harder to do unit testing.

@andrewlee94 Would you be able to give this PR another review. I've addressed most of your specific comments, and some of your above feedback has already been addressed. The rest can be covered in subsequent PR(s). Namely, the remaining action items are to add a demonstration/how-to, add a distinct test file for design_and_operation_model.py, and add more test(s) for methods in isolation

Copy link
Member

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

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

I am still not sure what some of the code here is for and why it is needed, and the existing docs do not really explain this. Thus, I am hesitant to approve this as-is in part because this suggests there might need to be more design discussion and secondly because with the existing documentation I am not confident of the maintainability of the code.

@declare_process_block_class("DesignModel")
class DesignModelData(ProcessBlockData):
"""
Class for containing design model ...
Copy link
Member

Choose a reason for hiding this comment

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

I still do not know enough about what this class actually does to approve this. The docs do not explain what this is or how to use it, and I am not sure what this code actually adds (i.e. is it even necessary?).

For one, this needs a descriptive doc string to explain what it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

@radhakrishnatg would be the best person to respond to this.

except:
have_skl = False
try:
from kneed import KneeLocator
Copy link
Member

Choose a reason for hiding this comment

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

Are these existing dependencies, or are they adding new dependencies we need to be able to handle? If these are new dependencies, then this needs to be discussed in more detail (i.e. a meeting).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@MarcusHolly MarcusHolly Oct 15, 2024

Choose a reason for hiding this comment

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

These are new dependencies. Linking to a previous discussion that explains the changes in setup.py: #1201 (comment). If there is a better way to go about this, let us know.

Copy link
Contributor

Choose a reason for hiding this comment

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

For optional dependencies, you can look into attempt_import() from pyomo.common.dependencies that streamlines the syntax a bit. You should be able to find usage examples in the IDAES codebase and/or Pyomo docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lbianchi-lbl Can you provide a bit more context about optional dependencies? For this model, having access to a k-means clustering function is necessary, so what would be the purpose of an attempt_import(). In other words, if the import attempt failed, we would just have to display an error message saying something along the lines of "Install dependency XYZ". In which case, would there really be any difference between optional dependencies and actual dependencies?

setup.py Outdated Show resolved Hide resolved
@blnicho blnicho removed the request for review from esrawli October 15, 2024 20:22
@MarcusHolly
Copy link
Contributor

@adam-a-a @radhakrishnatg Leaving a comment here so that we remember to discuss next week whether or not the scipy tools will suffice or if we want to go through the hurdle of adding additional dependencies with sklearn (scikit-learn)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DISPATCHES Priority:Normal Normal Priority Issue or PR WaterTAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.