Exawind / nalu-wind

Solver for wind farm simulations targeting exascale computational platforms
https://nalu-wind.readthedocs.io
Other
122 stars 83 forks source link

Converted to the new STK simple_fields workflow #1233

Closed djglaze closed 8 months ago

djglaze commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

@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.

psakievich commented 8 months ago

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.

djglaze commented 8 months ago

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...