amr-wind icon indicating copy to clipboard operation
amr-wind copied to clipboard

Fix input name for offset vector specification in the inputs

Open marchdf opened this issue 1 year ago • 3 comments

Summary

Fix input name for offset vector specification in the inputs because it was confusing users. The current optiion normal was renamed to offset_vector and an abort statement was added to warn users using the old option. The netcdf variable was also renamed to be more specific.

Pull request type

Please check the type of change introduced:

  • [ ] Bugfix
  • [ ] Feature
  • [ ] Code style update (formatting, renaming)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] Documentation content changes
  • [x] Other (please describe): user input file option

Checklist

The following is included:

  • [ ] new unit-test(s)
  • [ ] new regression test(s)
  • [x] documentation for new capability

This PR was tested by running:

  • the unit tests
    • [ ] on GPU
    • [x] on CPU
  • the regression tests
    • [ ] on GPU
    • [x] on CPU

Additional background

Closes #1132

marchdf avatar Jul 15 '24 15:07 marchdf

hi @marchdf, would it be possible to hold off on merging this PR until the next version release of amr-wind? We can do a couple of PSA's to let people know of the upcoming change, and we can synchronize this change with the corresponding change to amr-wind-frontend.

Also, just so it is completely obvious to people, we should include a diagram in the docs about what is the definition for axis1, axis2, offset_vector, etc.

Lawrence

lawrenceccheung avatar Jul 15 '24 15:07 lawrenceccheung

Thanks for pushing this change, Marc. The naming sounds good to me. I'm indifferent to when this gets merged but I agree that would be nice to let people know ahead of time-- maybe on @mbkuhn's recurring meeting regarding news/changes in amr-wind?

rthedin avatar Jul 15 '24 16:07 rthedin

Marking as draft until after the next major release. I added the following diagram to the docs:

planesampler

marchdf avatar Jul 15 '24 17:07 marchdf

@lawrenceccheung, we've moved to v3 of amr-wind since this PR was created and this was socialized I think. OK to merge now?

marchdf avatar Sep 05 '24 16:09 marchdf

Thanks @marchdf, we can include this. I think I know how to deal with this in the amr-wind-frontend so it doesn't break all of the old input files.

Just curious, are we keeping track of input file changes or additions between versions? At some point if we make lots of updates to the input parameters, people may want to know what input got added in which version of amr-wind (or also, which inputs are allowable in older amr-wind versions).

Lawrence

lawrenceccheung avatar Sep 05 '24 20:09 lawrenceccheung

We don't have a dependency tree between input arguments and different commits. However, the semantic versioning of the code (and release notes) is meant to communicate this. Also, we are being vigilant about keeping the input file reference up-to-date.

mbkuhn avatar Sep 05 '24 20:09 mbkuhn