-
Notifications
You must be signed in to change notification settings - Fork 525
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
add continue feature for depletion #3272
base: develop
Are you sure you want to change the base?
add continue feature for depletion #3272
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.
Looking for input on this line (636)
c7324fa
to
1b9dbbd
Compare
1b9dbbd
to
b5fe00b
Compare
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.
Just one fairly minor comment.
re-use variable and add missing range Co-authored-by: Edgar-21 <84034227+Edgar-21@users.noreply.github.com>
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.
Re-suggesting my comment from before since it was outdated by the commit of Edgar's suggestion. Might need to apply proper formatting for OpenMC, if applicable (should be black-compliant)
Co-authored-by: Connor Moreno <camoreno@wisc.edu>
Looks like it hit the
|
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 things to address here:
- We'll want to make sure that the call to
Results.get_times()
provides the same unis as the time steps passed to theIntegrator
class here. - The times coming from the
Results
object are points in time, notdt
, so we'll need to take the diff of that array to make the comparison to the time steps passed for the continuation run. - Similar to the
get_times
situation, the array coming from theResults.get_source_rates()
will include more values than the number of time steps and will need to be modified to make a valid comparison to the continuation source rates.
|
||
with pytest.raises( | ||
ValueError, | ||
match="You are attempting to continue a run in which the previous results do not have the same initial steps as those provided to the Integrator. Please make sure you are using the correct timesteps, powers or power densities, and previous results file.", |
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.
Let's wrap this string or use a subset that is sufficient to detect the error message.
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.
Let me know if the wrap looks good. For the subset method, do you just use an r-string and glob? I saw some other examples, so if you'd prefer, I can do something like this
with pytest.raises(
ValueError,
match = "You are attempting to continue a run in which the previous *"
):
bundle.solver(
operator, [0.75, 0.5, 0.75], [1.0, 1.0, 1.0], continue_timesteps=True
).integrate()
|
||
from tests import dummy_operator | ||
|
||
# test that the continue timesteps works when the second integrate call contains all previous timesteps |
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.
Let's make this a docstring for the new test
Possibly yes, but this is an error that can be reproduced locally easily enough. |
Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>
Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>
Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>
Turn out some of the tests were triggering the Here's my local version of the code # validate existing depletion steps are consistent with those passed to operator
if continue_timesteps:
completed_times = operator.prev_res.get_times(time_units=timestep_units)
completed_timesteps = completed_times[1:] - completed_times[:-1] # convert absolute t to dt
completed_source_rates = operator.prev_res.get_source_rates()
num_previous_steps_run = len(completed_timesteps)
if (np.array_equal(completed_timesteps, timesteps[:num_previous_steps_run])):
seconds = seconds[num_previous_steps_run:]
else:
print("timestep check failed")
print(f"timesteps given to continue run: {timesteps}")
print(f"found completed timesteps: {completed_timesteps}")
raise ValueError(
"You are attempting to continue a run in which the previous timesteps "
"do not have the same initial timesteps as those provided to the "
"Integrator. Please make sure you are using the correct timesteps."
)
if(np.array_equal(completed_source_rates, source_rates[:num_previous_steps_run])):
source_rates = source_rates[num_previous_steps_run:]
else:
print("source rate check failed")
print(f"SRs given to coninute run: {source_rates}")
print(f"completed SRs: {completed_source_rates}")
raise ValueError(
"You are attempting to continue a run in which the previous results "
"do not have the same initial source rates, powers, or power densities "
"as those provided to the Integrator. Please make sure you are using "
"the correct powers, power densities, or source rates and previous results file."
) I've added some print statements to get an idea of what the important variables look like during tests but I'm not totally sure why some tests are failing despite printing values that seem to be the same?
I tested the
I know we discussed some schemes might have more data in them than the |
As discussed today, I printed the non-sliced versions of the data above which clears up why the arrays appeared the same. E.g. we should actually do this to test if(np.array_equal(completed_source_rates, source_rates[:num_previous_steps_run])):
source_rates = source_rates[num_previous_steps_run:]
else:
print("source rate check failed")
print(f"Initial SRs given to coninute run: {source_rates[:num_previous_steps_run]}")
print(f"completed SRs: {completed_source_rates}") When I print what's actually compared in the if statements, we see
This is due to the fact that certain integrators with sub-steps write out more source rates than are initially provided. I will work on looking into how to capture this case and test appropriately. |
Description
This PR closes #2871. To summarize the motivation, I'd like depletion restarts to be a bit easier for users. If you set up a depletion simulation that doesn't end up getting run all the way (think hitting max wall time on an HPC or some other interrupt), the burden then falls to the user to correctly determine what time steps were not run and to create a new script that picks up where the other one left off. In my own experience, it is easy to make a mistake and re-run a simulation that is not what I desired.
All we do here is add a
continue_timesteps
flag (default toFalse
) that adds a new logic block to theIntegrator
class. It would be useful in the above case and requires users to provide theIntegrator
with a set oftimesteps
and one ofpower
/power_density
/source_rate
that matches what already exists in aprev_results
passed to theOpperator
.With this flag, the depletion restart python script can now be exactly the same as the initial script, just with a
continue_timesteps = True
flag and aprev_results
object loaded from adepletion_statepoint.h5
passed to theOperator
. It could contain more timesteps at the end too, but the only requirement is that the data matches what is in theOperator
provided to theIntegrator
for all the existing steps in the results.This PR does not eliminate any of the existing capability of the depletion API, so all the old use cases/syntax for restarts remain. The flag is optional and defaults to
False
so no one will need to update past scripts with this change.Checklist
I have run clang-format (version 15) on any C++ source files (if applicable)