Adding DeployInst Start Year Features (after and during simulation)#677
Adding DeployInst Start Year Features (after and during simulation)#677meg-krieg wants to merge 25 commits intocyclus:mainfrom
Conversation
Build Status Report - 8cb94d7 - 2026-02-04 18:01:19 +0000Build
|
|
Thanks for taking a shot at this @meg-krieg. It looks like you've taken an approach of allowing each facility to have a different deployment year. However, the intent of the issue - although perhaps not clear - was declare a single "year 0" for the entire This approach will provide much of the same functionality that your solution provides, but with a single additional and optional entry for the |
|
Thanks for the comment! I think I have updated it how you describe. There is only 1 deployyear accepted, and it offsets all the prototype build_times by the same number. I replaced the value warning with an error (mentioend in #674) for before simulation deployment and adjusted any unit tests. |
gonuke
left a comment
There was a problem hiding this comment.
This is definitely a better match to what I had in mind. Thanks @meg-krieg !!
A few suggestions here to simplify/streamline the code and add more tests.
| int sim_start = 12*(context()->sim_info().y0) + | ||
| context()->sim_info().m0; |
There was a problem hiding this comment.
This calculation can move outside/before the loop. In fact, it may be preferable to calculate
delta_t = deployyear*12 - sim_start
before the loop and then use that below.
| context()->sim_info().m0; | ||
| int t_build = build_times[i]; | ||
| if(deployyear > 0){ | ||
| if(t_build + deployyear*12 < sim_start){ |
There was a problem hiding this comment.
Or... if you have already modified t_build as suggested above:
| if(t_build + deployyear*12 < sim_start){ | |
| if(t_build < 0){ |
| int t = build_times[i]; | ||
| int sim_start = 12*(context()->sim_info().y0) + | ||
| context()->sim_info().m0; | ||
| int t_build = build_times[i]; |
There was a problem hiding this comment.
if delta_t as recommended above:
| int t_build = build_times[i]; | |
| int t_build = build_times[i] + delta_t; |
There was a problem hiding this comment.
Does this imply that start year should now be a required parameter? I left line t_build = build_times[i] before the loop for the scenario where the user does not specify the deployyear and t_build must remain a default.
If the t_build = build_times[i] does need to stay, I could still move some of the calculations out side of the for loop and adjust the if-statements as suggested for streamlining!
There was a problem hiding this comment.
Good catch... If deployyear is optional, then it will have a default value. We probably should think about a way to recognize that default and then set delta_t to 0 when the value is the default.
There was a problem hiding this comment.
It could be that we can set the default value as the simulation start year when we read that part of the input... but I'll have to think about that...
| else { | ||
| t_build += deployyear*12 - sim_start; | ||
| if(t_build >= context()->sim_info().duration) { |
There was a problem hiding this comment.
if you have already modified t_build as suggested above:
| else { | |
| t_build += deployyear*12 - sim_start; | |
| if(t_build >= context()->sim_info().duration) { | |
| elif(t_build >= context()->sim_info().duration) { |
| * Added new variable to Deploy Institution to shift the deployment times (#677) | ||
| * Added new tests for Deploy Institution year (#677) |
There was a problem hiding this comment.
These are probably better as a single entry since new tests are expected as part of any new addition
| * Added new variable to Deploy Institution to shift the deployment times (#677) | |
| * Added new tests for Deploy Institution year (#677) | |
| * Added new variable to Deploy Institution to shift the deployment times (#677) |
There was a problem hiding this comment.
can we add tests for the warning and the error?
Note - we have a pattern for testing for warnings where we use a method that temporarily treats warnings as exceptions
Summary of Changes
I added a optional deploy year variable to DeployInst. This variable will allow the user to input a year that informs DeployInst of when facilities should be built relative to the build times. It is essentially an offset to build time.
The current changes should anticipate whether the optional deploy year and build time exceed or are within the simulation duration. A value error should be thrown in the event the combination of deploy year and build time exceed simulation duration but the offset is still applied.
This PR does not yet address issue #674 but it does anticipate that users could input a deploy year and build time that is before the simulation start. In this scenario, the time of deployment is reset to time step 1 and a value error thrown highlighting this reset.
Design Notes
I kept deploy year as an optional parameter but this means that build time + deploy year combination must together be considered when evaluating when a facility should be built. For example, deploy year could be within the sim duration but build time offset makes it exceed duration. Or, deploy year could be before sim duration but build time offset makes it within the simulation.
Check the box if your change does not break any of the following:
Related CEPs and Issues
Fixes #676 and #675
Testing and Validation
Added two tests. One tests whether build time + deploy year falls on the expected year with in the simulation and deploys. Another test checks whether or not build time + deploy year falls before the simulation start and that the deploy times are properly reset to simulation start.
As an aside, I am slightly unsure about the conversion from year to time-step in the code changes.
Reviewers, please refer to the Cyclus Guide for Reviewers.