search

arfc / transition-scenarios

A repository to hold transition scenarios with Cyclus.
Other
3 stars 12 forks source link

Reactor Deployment Script #168

Closed nsryan2 closed 2 months ago

nsryan2 commented 2 months ago

This PR creates a new script in the repo that calculates when and how many reactors should be deployed with 4.5 deployment schemes.

Deployment Functions

  1. Greedy Algorithm: deploy the largest reactor first at each time step, fill in the remaining capacity with the next smallest, and so on.
  2. Pre-determined distributions: one or more reactors have a preset distribution, and a smaller capacity model fills in the gaps.
    1. Deployment Cap [extension of 2]: there is a single-number capacity for one or more of the reactor models. There is no function for this, just use a constant distribution.
  3. Random Deployment: uses a date and hour as seed to randomly sample the reactors list.
  4. Initially Random, Greedy: randomly deploys reactors until a reactor bigger than the remaining capacity is proposed for each year, then fills remaining capacity with a greedy algorithm.

There are corresponding tests and documentation in the style of the repository.

pep8speaks commented 2 months ago

Hello @nsryan2! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 8:1: E402 module level import not at top of file

Comment last updated at 2024-08-30 17:40:58 UTC
nsryan2 commented 2 months ago

I'm ignoring the pep8speaks comment about the module level import because the path needs to be specified in the tests to allow for importing the deployment_script (as with the other tests in this repo). This issue is documented in #148.

nsryan2 commented 2 months ago

Thanks for the suggestions @LukeSeifert, I see what you mean about a class. I'll reconfigure to incorporate that change.

I'll say, it's not how the rest of the scripts are setup and that's probably to their detriment. This feedback would be good to connect with #58

LukeSeifert commented 2 months ago

Thanks for the suggestions @LukeSeifert, I see what you mean about a class. I'll reconfigure to incorporate that change.

I'll say, it's not how the rest of the scripts are setup and that's probably to their detriment. This feedback would be good to connect with #58

I see what you mean, maybe it would be worth putting that off until a full refactor. However you decide, let me know once you're ready for me to take another look.

nsryan2 commented 2 months ago

@LukeSeifert thanks for the suggestions on improving the code. I have implemented all of them except for the class idea. I agree that it would improve this code, but I am hesitant to make the change now without thinking more broadly about the other scripts in this repo. I will make a note of this in the existing issue and start thinking about those changes.