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

Tupek/test quasi static checkpoint #1183

Merged
merged 17 commits into from
Aug 9, 2024
Merged

Conversation

tupek2
Copy link
Collaborator

@tupek2 tupek2 commented Jul 20, 2024

The checkpointing was not being done for quasi-static problems. Now, it is written a bit more generically with a bit more responsibility on base_physics and loops over states so that it will work for any physics and dynamic or quasi-static. In addition, we were not decrementing the time correctly, so any time dependent load was not being accounted for in the reverse pass correctly. This tries to clear up some of the logic and naming, and fix these know failing situations. Tests were added to check time varying loads and quasi-static evolution.

Additional things in this: remove ode_time_point_, as it was very confusingly intertwined with time_. If there is a case to keep both, we can undo this. Also, the checkpoint to disk capability is a bit subtle. Now it is always writing to disk. If the user externally also writes state to disk, it will mess up the checkpointing. I think we can considered deleting the writing to disk checkpointing altogether at some point. For now, it is fairly hard to use.

@tupek2 tupek2 requested a review from btalamini July 20, 2024 14:19
@tupek2 tupek2 force-pushed the tupek/test-quasi-static-checkpoint branch 3 times, most recently from 442e66c to 8d98cb6 Compare August 1, 2024 04:20
@@ -346,7 +343,8 @@ class HeatTransfer<order, dim, Parameters<parameter_space...>, std::integer_sequ
} else {
// Step the time integrator
// Note that the ODE solver handles the essential boundary condition application itself
ode_.Step(temperature_, time_, dt);
double time_tmp = time_;
ode_.Step(temperature_, time_tmp, dt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you make a copy of time_ before passing to ode_?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time_ is already passed to the constructor of the time integrator. It is already incremented there. This value would also be incremented. I want to make sure the incrementing wasnt happening twice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put a comment of why you are doing this here and below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added words.

@kswartz92
Copy link
Contributor

are you planning to add the test with a Functional qoi to this PR?

@tupek2 tupek2 force-pushed the tupek/test-quasi-static-checkpoint branch from 79117d3 to 4daaea7 Compare August 2, 2024 01:29
@tupek2 tupek2 added the ready for review Ready for active inspection by reviewers label Aug 2, 2024
@tupek2
Copy link
Collaborator Author

tupek2 commented Aug 2, 2024

are you planning to add the test with a Functional qoi to this PR?

added

@kswartz92
Copy link
Contributor

Update- Irvin pulled the latest changes from this branch into his lido build and his problem has correct sensitivities and a working inverse problem with time dependent b.c.s and time dependent QoI. Thanks for all the help @tupek2!

@tupek2 tupek2 added WIP Work in progress and removed ready for review Ready for active inspection by reviewers labels Aug 6, 2024
@tupek2 tupek2 force-pushed the tupek/test-quasi-static-checkpoint branch from e7f4006 to c39ce0b Compare August 6, 2024 19:42
@@ -1352,9 +1354,21 @@ class SolidMechanics<order, dim, Parameters<parameter_space...>, std::integer_se
mfem::HypreParVector adjoint_essential(displacement_adjoint_load_);
adjoint_essential = 0.0;

SLIC_ERROR_ROOT_IF(cycle_ <= min_cycle_,
"Maximum number of adjoint timesteps exceeded! The number of adjoint timesteps must equal the "
"number of forward timesteps");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be helpful here to mention to users that this message can be caused by two backwards passes attempted without a forward pass or a clearAdjointStates() in between

@tupek2 tupek2 force-pushed the tupek/test-quasi-static-checkpoint branch from 1321cee to cb1d924 Compare August 8, 2024 15:31
Copy link
Contributor

@samuelpmishLLNL samuelpmishLLNL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay reviewing this, I was hoping to grok it more thoroughly but it sounds like it's holding other things up.

@@ -57,7 +57,7 @@ double BasePhysics::minTime() const { return min_time_; }

int BasePhysics::minCycle() const { return min_cycle_; }

std::vector<double> BasePhysics::timesteps() const { return timesteps_; }
const std::vector<double>& BasePhysics::timesteps() const { return timesteps_; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

double BasePhysics::getCheckpointedTimestep(int cycle) const
{
SLIC_ERROR_ROOT_IF(cycle < 0, axom::fmt::format("Negative cycle number requested for physics module {}.", name_));
SLIC_ERROR_ROOT_IF(cycle > max_cycle_,
SLIC_ERROR_ROOT_IF(cycle > static_cast<int>(timesteps_.size()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this error check consistent with the message but should we have a check that we don't go over max_cycle_?

@tupek2 tupek2 force-pushed the tupek/test-quasi-static-checkpoint branch from cb1d924 to def2e91 Compare August 8, 2024 23:13
@white238
Copy link
Member

white238 commented Aug 9, 2024

/style

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants