-
Notifications
You must be signed in to change notification settings - Fork 7
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
Logical flow #71
Logical flow #71
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I just had a couple comments on some docstrings and the MWh
usage. I also had some trouble running the tests myself. The tests fail with installation method 1 (from source) and method 2 for me after I cloned your repo and switched to the logical_flow
branch. Do you know what I need to do to test this version of osier
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @samgdotson, great work on this PR. A couple items
- Docpages have not been updated to hold the new classes
- Line-items (see comments)
Additionally, it's not clear to me where the no-solver change is, nor where parallelization of the genetic algorithm is enabled. If you could describe these changes in a bit more detail that'd be wonderful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reviews @yardasol and @LukeSeifert!
@yardasol, just addressing your comments/questions:
The docs should be updated, now. Also, I'm not sure why the GH actions are failing, but it's not anything related to the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I made a comment on one docstring that has the parameters missing, everything else seems good to me
@LukeSeifert @yardasol if you're satisfied with the changes feel free to merge (in spite of the failing checks -- I think it is a GH issue). |
Okay, I think the issue is because Mambaforge is no longer being maintained, so the installer is looking for a link that doesn't go anywhere, anymore. See this release. Working on a fix. |
@yardasol, checks pass, now. Are you satisfied with the changes? @LukeSeifert, regarding testing locally, I can't say for sure what's wrong because I haven't seen the error message. However, I speculate that the errors might be because you haven't "installed" the branch. Try creating an editable installation (as described in the contributing guide) with python -m pip install -e .[doc] This way, you can switch between branches at will and the "installed version" of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good changed @samgdotson. I found one little snag but other than that we're good to go!
Co-authored-by: Olek <[email protected]>
Thanks @yardasol! Feel free to merge whenever! |
Summary of changes
This PR adds a new dispatch model to
osier
calledLogicDispatchModel
that follows a hierarchy of rules to dispatch energy. It is much faster (~4x speedup) than the originalDispatchModel
but it is myopic and therefore results may be sub-optimal. Other benefits include:osier
is more self-contained.DispatchModel
. No change to syntax.CapacityExpansion
model.Note
There are a couple of new tests for new functionality. These tests should exceed the minimum code coverage required by the
pyproject.toml
file (75%). More tests are desired, but not necessary, here.Edit: Closes #54 by changing
MW*hr
toMWh
.Types of changes
Checklist for Reviewers
Reviewers should use this link to get to the
Review Checklist before they begin their review.