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

Converted to the new STK simple_fields workflow #1237

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

djglaze
Copy link
Contributor

@djglaze djglaze commented Feb 8, 2024

STK is migrating to a new strategy for registering and managing Fields, where sizing information is purely specified at run-time instead of the previous technique of specifying it in a confusing blend of both compile-time and run-time information. The compile-time specification was just a suggestion, as it could be overridden (possibly inconsistently) at run-time to support variable-length Fields. This made it unclear what the true size of a Field was and where it should be specified.

As an example, registering a vector field on the entire mesh previously looked like this:

using VectorField = stk::mesh::Field<double, stk::mesh::Cartesian3d>;
VectorField & field = meta.declare_field(stk::topology::NODE_RANK, "velocity");
stk::mesh::put_field_on_mesh(field, meta.universal_part(), 3, nullptr);

and now, it looks like this:

using VectorField = stk::mesh::Field;
VectorField & field = meta.declare_field(stk::topology::NODE_RANK, "velocity");
stk::mesh::put_field_on_mesh(field, meta.universal_part(), 3, nullptr);

stk::io::set_field_output_type(field, stk::io::FieldOutputType::VECTOR_3D); // Optional

The only template parameter for a Field is now the datatype parameter. Sizing information now exclusively comes from put_field_on_mesh() calls. The optional set_field_output_type() function call registers with the IO sub-system how a multi-component Field should be subscripted in Exodus files. If this call is left off, you will get the default [_1, _2, _3] subscripting. With the above call, you will instead get [_x, _y, _z] subscripting.

The MetaData::use_simple_fields() flag is set everywhere possible in the code to prevent accidental regressions before the old behavior is formally deprecated and removed. This will yield a run-time error if the old-style extra template parameters are used anywhere. These calls to use_simple_fields() can be removed in the future once the STK Mesh back-end has removed support for the old behavior.

This wasn't a completely straightforward conversion due to nalu-wind making heavy use of various algorithm selections based on the templated Field type. The ScalarFieldType, VectorFieldType, TensorFieldType, and GenericFieldType types are now all identical, so different techniques had to be used to switch behaviors.

@djglaze
Copy link
Contributor Author

djglaze commented Feb 8, 2024

@psakievich This commit doesn't conflict with your commit #1236, so there isn't an order dependence for merging. This has the bug from last time fixed.

I agree with your comment in #1236, where it would make sense to change the SmartField retrieval template parameter to something like: fieldManager_.get_legacy_smart_field<double>("temperature") instead of the current fieldManager_.get_legacy_smart_field<sierra::nalu::ScalarFieldType>("temperature"). After this simple_fields change, the Field typedefs no longer convey any useful meaning beyond just the datatype. Using ScalarFieldType vs. VectorFieldType would yield identical results, which is a bit confusing.

@psakievich
Copy link
Contributor

This looks good to me. I will do another pass later this afternoon. @djglaze would you mind pointing out what the issue was for the intel compiler. Lot's of code to look over here. @jrood-nrel would you mind building this on eagle and confirming that the issue is resolved? I am in the process of revamping the SNL configs and all the other spack-manager/exawind-manager changes so it would be a big help. We don't have a clean intel environment at the moment on SNL hardware.

@djglaze
Copy link
Contributor Author

djglaze commented Feb 8, 2024

@psakievich The issue with the intel compiler was at src/WallDistEquationSystem.C, line 71. The constructor for the nodalGradAlgDriver_ now needs to know about both the input and output Fields, and I goofed on the name of the new input Field argument. This led to the code querying sizing information from a null Field. I've double-checked all other similar code changes, and this was the only place where I messed it up.

@djglaze
Copy link
Contributor Author

djglaze commented Feb 21, 2024

This commit was updated to work with some recently-merged nalu-wind commits that introduced more old-style Field code. There is a large PR up right now that also introduces incompatible code, that will also be a problem. I can patch this up again once that gets merged in.

@psakievich also, the CI process here appears to have failed with a low-level spack-manager scripting error. I'm not sure what to do about that.

@psakievich psakievich enabled auto-merge (squash) March 14, 2024 15:14
@psakievich
Copy link
Contributor

@djglaze thanks for your patience. I am going to get this merged and the other PR will have to update. I'm hoping to attack the smart fields as a background task as soon as this is merged. CI is broken but it is a one line fix for the group making our containers. Once that is done I'll retrigger the github actions and merge this.

Copy link
Contributor

@psakievich psakievich left a comment

Choose a reason for hiding this comment

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

I have been too busy to test as much as I would like, but it looks fine without testing. I'm going to merge it and if we have any build issues we can revert.

@djglaze
Copy link
Contributor Author

djglaze commented Mar 14, 2024

Thanks, this sounds great to me! If the other massive "Rebase Multiphase_Dev" PR isn't fixed to adapt to the simple_fields changes, then if it merges correctly, it's not clear to me that its new unit tests go deep enough to trip any of the new code that will now throw. So, either it will be a pre-existing unit test that might flag an issue during CI testing, or it will cause run-time failures that will only be found after merging.

@psakievich
Copy link
Contributor

@djglaze got the CI up and going again. Note there are some build errors.

STK is migrating to a new strategy for registering and managing
Fields, where sizing information is purely specified at run-time
instead of the previous technique of specifying it in a confusing
blend of both compile-time and run-time information.  The compile-time
specification was just a suggestion, as it could be overridden
(possibly inconsistently) at run-time to support variable-length
Fields.  This made it unclear what the true size of a Field was
and where it should be specified.

As an example, registering a vector field on the entire mesh
previously looked like this:

  using VectorField = stk::mesh::Field<double, stk::mesh::Cartesian3d>;
  VectorField & field = meta.declare_field<VectorField>(stk::topology::NODE_RANK, "velocity");
  stk::mesh::put_field_on_mesh(field, meta.universal_part(), 3, nullptr);

and now, it looks like this:

  using VectorField = stk::mesh::Field<double>;
  VectorField & field = meta.declare_field<double>(stk::topology::NODE_RANK, "velocity");
  stk::mesh::put_field_on_mesh(field, meta.universal_part(), 3, nullptr);

  stk::io::set_field_output_type(field, stk::io::FieldOutputType::VECTOR_3D); // Optional

The only template parameter for a Field is now the datatype parameter.
Sizing information now exclusively comes from put_field_on_mesh() calls.
The optional set_field_output_type() function call registers with the
IO sub-system how a multi-component Field should be subscripted in
Exodus files.  If this call is left off, you will get the default
[_1, _2, _3] subscripting.  With the above call, you will instead get
[_x, _y, _z] subscripting.

The MetaData::use_simple_fields() flag is set everywhere possible in
the code to prevent accidental regressions before the old behavior
is formally deprecated and removed.  This will yield a run-time error
if the old-style extra template parameters are used anywhere.  These
calls to use_simple_fields() can be removed in the future once the
STK Mesh back-end has removed support for the old behavior.

This wasn't a completely straightforward conversion due to nalu-wind
making heavy use of various algorithm selections based on the
templated Field type.  The ScalarFieldType, VectorFieldType,
TensorFieldType, and GenericFieldType types are now all identical,
so different techniques had to be used to switch behaviors.
auto-merge was automatically disabled March 15, 2024 17:06

Head branch was pushed to by a user without write access

@djglaze
Copy link
Contributor Author

djglaze commented Mar 15, 2024

Thanks for the heads-up, @psakievich . I guess I didn't build and test it locally with Tioga turned on. It should be good now.

@psakievich psakievich merged commit aa0617e into Exawind:master Mar 15, 2024
3 checks passed
jrood-nrel added a commit that referenced this pull request Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants