-
Notifications
You must be signed in to change notification settings - Fork 35
Gretl submodule #1495
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
base: develop
Are you sure you want to change the base?
Gretl submodule #1495
Conversation
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
…_state_advancer.hpp Co-authored-by: Chris White <[email protected]>
…_state_advancer.hpp Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
Co-authored-by: Chris White <[email protected]>
btalamini
left a comment
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.
The integration looks very close to what I was expecting, so LGTM.
|
|
||
| namespace smith { | ||
|
|
||
| /// @brief uses the constrained docs on the bc_manager to zero the corresponding dofs in FieldState s. |
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.
| /// @brief uses the constrained docs on the bc_manager to zero the corresponding dofs in FieldState s. | |
| /// @brief uses the constrained dofs on the bc_manager to zero the corresponding dofs in FieldState s. |
| return smith::tuple{get<VALUE>(Rho), tensor<double, spatial_dim>{}}; | ||
| } else { | ||
| auto ones = make_tensor<lumped_dim>([](int) { return 1.0; }); | ||
| return smith::tuple{-get<VALUE>(Rho) * ones, tensor<double, lumped_dim, spatial_dim>{}}; |
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.
The fact that only the singleton case is negated is making me suspicious of a bug. Is this intentional?
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.
its been too long. Probably it wouldn't compile without it. I mean, the ranks of the tensors are different. Maybe there is some nicer way to handle it, but I can't see it looking at it now.
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.
I see, there was a bug, that was compensated for elsewhere. Fixed in an update.
| namespace smith { | ||
|
|
||
| /// Evaluates a DoubleState using a provided ScalarObjective instance, and the input arguments to that objective. This | ||
| /// operation is tracked on the gretl graph. ScalarObjective must remain in scope for 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.
looks like the last word of the comment got cut off.
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.
Fixing
| DoubleState evaluateObjective(std::shared_ptr<ScalarObjective> objective, const TimeInfo& time_info, | ||
| const FieldState& shape_disp, const std::vector<FieldState>& inputs); | ||
|
|
||
| /// operation is tracked on the gretl graph. |
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.
Doc comment looks cut off. Missing the "@brief"
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.
fixed
| auto ke = 0.5 * rho * inner(v, v); | ||
| auto dx_dX = get<DERIVATIVE>(U) + Identity<dim>(); | ||
| auto J = det(dx_dX); | ||
| return ke * J; |
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.
Why is this multiplied by J? Rho is the density in the reference config, right? If so, no J.
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.
well, technically, this is not a shape-aware functional implementation (this is like a 10 months old implementation at this point). So, that argument is the shape-displacement (or the displacement if the current density is passed in, e.g., for updated Lagrange things). Perhaps in a follow up PR, I can convert this to using the new ScalarObjective interface which is shape-aware always.
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.
Is your tribol submodule out of date?
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.
there was an issue in tribol where if you didn't build with umpire, some part of its build would fail, so a patch was needed.
chapman39
left a comment
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.
i know about the tribol issue, but was there something wrong with the axom and mfem submodules as well that required an update? that'd be useful to me to know so next time i update ill make sure those changes are included.
I had temporarily accidentally not merged the updated axom and mfem from dev, but I think that is fixed now? Unless I am not reading the diffs correctly? |
|
|
||
| if(SMITH_ENABLE_TESTS) | ||
| add_subdirectory(tests) | ||
| endif() No newline at end of 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.
| endif() | |
| endif() | |
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.
Is there a way our make style can just do this for us automatically? Its sort of time consuming for us to constantly check it in PRs and have to update file by file.
| ) | ||
|
|
||
| blt_add_library( | ||
| NAME smith_differentiable_numerics |
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.
is differentiable_numerics something that would be better moved to a subdirectory of numerics?
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.
I think my naming may have caused confusion here. The include dependencies make it clear that differentiable_numerics does not belong in numerics. It combines things in physics, numerics, gretl, etc. to create analogous structures as exist in numerics, but which will be differentiated on the gretl graph. Its not a perfect match to numerics, so perhaps a name change could avoid confusion. differentiable_utilities was another option I was considering. If people have suggestions, I'm happy to change it, but preferably in a follow on PR.
i see that now. ok, nevermind then! besides my other question lgtm |
This adds gretl as a submodule and provides a first demo using FieldState (gretl-wrapped FiniteElementState and FiniteElementDual) for an explicit dynamics problem with dynamic checkpointing and back-propagation. Some minor differentiable_physics interface adjustments as well. Updating tribol to the latest develop branch to address a somewhat localized issue with builds that do not enable umpire.