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

implement random a/d buypol in storage #568

Merged
merged 17 commits into from
Feb 4, 2024

Conversation

nuclearkatie
Copy link
Contributor

@nuclearkatie nuclearkatie commented Jan 17, 2024

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

@nuclearkatie nuclearkatie marked this pull request as ready for review January 23, 2024 01:38
@nuclearkatie
Copy link
Contributor Author

@gonuke merged in the line from #570. Unfortunately, now there's a new CI error when it goes to run the cycamore python tests (the unit tests pass now):
ModuleNotFoundError: No module named 'cyclus'

@gonuke
Copy link
Member

gonuke commented Jan 24, 2024

@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.

@bennibbelink
Copy link
Contributor

bennibbelink commented Jan 25, 2024

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

@bennibbelink
Copy link
Contributor

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

@nuclearkatie
Copy link
Contributor Author

Thanks @bennibbelink !

Copy link
Member

@gonuke gonuke left a 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

src/storage.cc Outdated Show resolved Hide resolved
src/storage.cc Outdated Show resolved Hide resolved
src/storage.cc Outdated Show resolved Hide resolved
src/storage.cc Outdated Show resolved Hide resolved
src/storage.cc Outdated Show resolved Hide resolved
src/storage.h Outdated Show resolved Hide resolved
src/storage.h Outdated Show resolved Hide resolved
src/storage.h Show resolved Hide resolved
src/storage.h Outdated Show resolved Hide resolved
src/storage.h Show resolved Hide resolved
@nuclearkatie
Copy link
Contributor Author

@gonuke I think I addressed all your comments

Copy link
Member

@abachma2 abachma2 left a 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.

src/storage.cc Show resolved Hide resolved
src/storage.h Show resolved Hide resolved
Copy link
Member

@gonuke gonuke left a 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.

src/storage.cc Outdated Show resolved Hide resolved
@gonuke
Copy link
Member

gonuke commented Feb 4, 2024

Thanks for this great addition @nuclearkatie

@gonuke gonuke merged commit 2fe80d8 into cyclus:main Feb 4, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

demonstrate randomness in Storage (using toolkit)
4 participants