-
Notifications
You must be signed in to change notification settings - Fork 40
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
implement random a/d buypol in storage #568
Conversation
@bennibbelink - can you shed any light on why we aren't finding the cyclus python module on the most recent image? I'm not longer sure which CI path generated the image. |
4ab56e4
to
808537a
Compare
I found the issue, it has to do with the way we are finding the site-packages directory in the cycamore workflow (which is broken now that we don't build cyclus as an egg). I fixed it in this PR, but I can make it a separate PR to follow best practices. Also... the fix is still a little hacky. I will try to find a more robust solution |
Tried to make a different PR but realized that CI won't pass without the changes you've added here, so I think it's best to just include this fix in this PR. I modified it to be a bit more robust than it was before |
Thanks @bennibbelink ! |
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.
A few additional comments here now that I look at the state variables carefully, with their comments
857afe1
to
312586e
Compare
@gonuke I think I addressed all your comments |
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 @nuclearkatie. This looks pretty good overall, and it looks like you addressed @gonuke's comments, from what I could see. I have a few more comments for you, mostly for my understanding.
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.
One really small change - I really didn't want to delay further, but it's driven by our code readability standards.
Thanks for this great addition @nuclearkatie |
Requires cyclus#1634 to be merged first! (merged 1/17/24). Note that cyclus#1634 breaks cycamore storage because it changes the active dormant API. Until this PR is merged, the current cycamore/main storage archetype will not work with cyclus/main!
Closes #553 and #555
This PR makes the full range of fixed and random active and dormant and size buying behaviors available to Storage. To do so, it introduces a type and five numeric values each for the active frequency, dormant frequency, and request size (18 new parameters total).
Testing included