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 #1233

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Converted to the new STK simple_fields workflow #1233

merged 1 commit into from
Dec 18, 2023

Conversation

djglaze
Copy link
Contributor

@djglaze djglaze commented Dec 11, 2023

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.

@djglaze djglaze requested a review from psakievich December 11, 2023 23:41
@djglaze
Copy link
Contributor Author

djglaze commented Dec 11, 2023

Arrrgh. How do I see which lines the format checker is unhappy with? When I click on the "Details" link, it appears to just echo back at me the entire diff of this (massive) commit. I put significant effort into wrapping each change to 80 columns in the same style that the clang formatter appeared to be using.

@alanw0
Copy link
Contributor

alanw0 commented Dec 11, 2023

Arrrgh. How do I see which lines the format checker is unhappy with? When I click on the "Details" link, it appears to just echo back at me the entire diff of this (massive) commit. I put significant effort into wrapping each change to 80 columns in the same style that the clang formatter appeared to be using.

That big diff when you click on details does seem to be the clang formatter output.
Let me see if I still have a recipe for running the clang formatter. If not, Phil no doubt has a recipe for it.

@djglaze
Copy link
Contributor Author

djglaze commented Dec 12, 2023

Arrrgh. How do I see which lines the format checker is unhappy with? When I click on the "Details" link, it appears to just echo back at me the entire diff of this (massive) commit. I put significant effort into wrapping each change to 80 columns in the same style that the clang formatter appeared to be using.

That big diff when you click on details does seem to be the clang formatter output. Let me see if I still have a recipe for running the clang formatter. If not, Phil no doubt has a recipe for it.

Thanks. That would be helpful. I've somehow managed to never run afoul of the format checker, and I've never run the clang formatter before.

@alanw0
Copy link
Contributor

alanw0 commented Dec 12, 2023

It looks like you can run clang-format using something like this:
find . -iname *.h -o -iname *.C | xargs clang-format -i
@psakievich do you have the path to the version of clang you're favoring these days?

@djglaze
Copy link
Contributor Author

djglaze commented Dec 12, 2023

It looks like you can run clang-format using something like this: find . -iname *.h -o -iname *.C | xargs clang-format -i @psakievich do you have the path to the version of clang you're favoring these days?

Thanks, that worked. I used clang-14, and the format checker here gobbled it up just fine. I disagree with almost all of the changes it made, but whatever.

@djglaze
Copy link
Contributor Author

djglaze commented Dec 12, 2023

On to the next issue... I monkeyed with the code in the wind-utils.git submodule, but the CI is complaining that it can't find the SHA of those changes. Do I need to do a separate PR for that submodule repo before this PR can go through?

@psakievich
Copy link
Contributor

@djglaze there is a decorator
// clang-format off (or on)

You can use to disable formatting in areas it makes it harder to read.

You can also copy the diff to a text file and then git apply the diff to pass the formatter.

I'll take a look at the code tonight or tomorrow.

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.
@psakievich
Copy link
Contributor

wow this is a big PR. Must have been a lot of work @djglaze. I can't say I've gone line by line through the PR but I like the new stk syntax a lot better.

@psakievich psakievich merged commit 8e8f4d5 into Exawind:master Dec 18, 2023
3 checks passed
@djglaze
Copy link
Contributor Author

djglaze commented Dec 19, 2023

wow this is a big PR. Must have been a lot of work @djglaze. I can't say I've gone line by line through the PR but I like the new stk syntax a lot better.

Yeah, @psakievich, this is a bigger PR than I would have liked. I wouldn't want to review it. :-) So, thanks! It took about a month of background-mode work. If the changes were done correctly, then there should be precisely no behavior difference. All regression and unit tests run the same, so I think we're in the clear.

I'm not exactly sure how the whole submodule thing works, but this change references a SHA that's up for review in the wind-utils repo. It's just a (much) smaller version of this kind of change. I'm a little surprised that this commit could be merged before the wind-utils commit...

jrood-nrel added a commit that referenced this pull request Jan 5, 2024
jrood-nrel added a commit that referenced this pull request Jan 5, 2024
psakievich added a commit that referenced this pull request Apr 30, 2024
* Add Timers for FSI (#1221)

* Add Timers for FSI

* Style

* Add FSI section to Input File docs. (#1220)

* Add FSI section to Input File docs.

* Updating the FSI docs to incorporate PR comments.

* Relax projection for FSI mapping to account for curvature (#1223)

* Relax projection for FSI mapping to account for curvature

* Style

* Format again

* Add option to dump mesh on failed jacobian check (#1226)

* Add option to dump mesh on failed jacobian check

* Style

* Cleaner, parallel consistent impl

* Correctly set current_coordinates for restart (#1227)

* Actually set current_coordinates for restart

* Remove redundant call updating displacements

* Revert "Remove redundant call updating displacements"

This reverts commit dca015c.

* Fix restart issue for FSI simulations (#1228)

---------

Co-authored-by: Ganesh Vijayakumar <[email protected]>

* Update FieldRegistry.h (#1229)

missing std::.

* FSI: Set ramping defaults to false (#1231)

* FSI: Set ramping defaults to false

Since we are moving to split meshes as the current strategy setting
ramping defaults to `False`.

I have been investigating why the temporal ramping is causing
simulations to fail since the bug fixes regarding hub motion, and I
can't find any issues with it at the moment. So it seems best to keep
that turned off as well.

* Update real defaults

* add a string-function temperature IC/dirichlet bc (#1198)

* add a string-function temperature IC/dirichlet bc

* undo stk deprecation fixes

---------

Co-authored-by: psakievich <[email protected]>

* Converted to the new STK simple_fields workflow (#1233)

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.

* Fixes errors in ghosting update for some overset mesh problems. (#1214)

Instead of trying to modify the ghosting for overset problems with
relative motion, we now just rebuild the ghosting from scratch.
This appears to fix errors such as those reported in #936

Co-authored-by: dcdemen <[email protected]>
Co-authored-by: psakievich <[email protected]>

* Revert "Converted to the new STK simple_fields workflow (#1233)" (#1234)

This reverts commit 8e8f4d5.

* Fixed the iblank inconsistency at shared nodes. The iblank field is returned to Tioga after the stk::mesh::copy_owned_to_shared operation, prior to field interpolation (#1238)

* Multiphase milestone (#1222)

* Divide through by density to get velocity form

* Fix VOF velocity flux to finalize

* Fix missing pressure gradient density norm and sharpen interfaces more

* Add diffusion term to VOF

* center droplet and provide velocity

* first version of sloshing tank

* sloshing tank case, can change parameters

* Mass-momentum consistency with initial vel scale and diffusion values

* Remove unused forced mass flux from pressure equation

* clean-up and safe settings for VOF advection

* Density face definition for minimized buoyancy noise

* rho_ref == initial density

* Change initial density to a specified function and introduce wall bcs for VOF

* sloshing tank pressure profile

* user function for generic flat water level in z

* Cleaning

* Balanced Buoyancy Forcing

* Complete milestone implementation

* Full implementation with fixed testing

* Fix allocation of fields

* Formatting

* Documentation of VOF advection scheme

---------

Co-authored-by: whorne <[email protected]>
Co-authored-by: Michael Kuhn <[email protected]>

* Rebase and pressure gradient improvements for VOF

* Fixed interface width work

* Allow slip of VOF at wall bcs

---------

Co-authored-by: psakievich <[email protected]>
Co-authored-by: neilmatula <[email protected]>
Co-authored-by: Ganesh Vijayakumar <[email protected]>
Co-authored-by: 四月是你的谎言 <[email protected]>
Co-authored-by: rcknaus <[email protected]>
Co-authored-by: djglaze <[email protected]>
Co-authored-by: ddement <[email protected]>
Co-authored-by: dcdemen <[email protected]>
Co-authored-by: Jon Rood <[email protected]>
Co-authored-by: itopcuoglu <[email protected]>
Co-authored-by: whorne <[email protected]>
Co-authored-by: Michael Kuhn <[email protected]>
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.

3 participants