Skip to content

Add u,v and biogeochem variables to the vertical remapping#912

Merged
travissluka merged 6 commits intodevelopfrom
feature/remap_uv
Jul 13, 2023
Merged

Add u,v and biogeochem variables to the vertical remapping#912
travissluka merged 6 commits intodevelopfrom
feature/remap_uv

Conversation

@guillaumevernieres
Copy link
Contributor

@guillaumevernieres guillaumevernieres commented Jun 27, 2023

Description

Added u, v, chl, biop, and po4 to the possible remapping cases. This assumes that they are defined on the tracer grid, which isn't the case for (u, v) when reading the model native fields. Use at your own risk!

Issue(s) addressed

Testing

How were these changes tested? part of the ci
What compilers / HPCs was it tested with? gnu
Are the changes covered by ctests?

Dependencies

If testing this branch requires non-default branches in other repositories, list them.
Those branches should have matching names for the automated TravisCI tests to pass

Do PRs in upstream repositories need to be merged first?
If so add the "waiting for other repos" label and list the upstream PRs

  • waiting on JCSDA/soca-bundle/pull/<pr_number>
  • waiting on JCSDA/oops/pull/<pr_number>

@guillaumevernieres
Copy link
Contributor Author

@liuxiao37k , should we add your biogeochem tracers as well?

@liuxiao37k
Copy link
Contributor

@liuxiao37k , should we add your biogeochem tracers as well?

Aha!! Could this at least partially explain why BGC analysis was so messed up in the hybrid configuration?

Yes, if you could please add chl, biop, and po4 to the fix, that would be great! Thanks! @guillaumevernieres

@guillaumevernieres
Copy link
Contributor Author

@liuxiao37k , should we add your biogeochem tracers as well?

Aha!! Could this at least partially explain why BGC analysis was so messed up in the hybrid configuration?

Yes, if you could please add chl, biop, and po4 to the fix, that would be great! Thanks! @guillaumevernieres

No idea if this will fix your issues @liuxiao37k but I'll add the fields above.

@guillaumevernieres guillaumevernieres changed the title Add u,v to vertical remapping Add u,v and biogeochem variables to the vertical remapping Jun 27, 2023
Copy link
Contributor

@travissluka travissluka left a comment

Choose a reason for hiding this comment

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

you're not going to like this, but I would prefer if this were done a different way. Odds are we'll add another state variable in the future, forget to add it here, and not know what's going on.

I think the more future-proof fix would be to add a parameter to the fields metadata for "vert interp" or something like that, set it to true by default for 3D fields, and then explicitly set it to false in the fields metadata yaml for the fields we know should not be interpolated.

@guillaumevernieres
Copy link
Contributor Author

you're not going to like this, but I would prefer if this were done a different way. Odds are we'll add another state variable in the future, forget to add it here, and not know what's going on.

I think the more future-proof fix would be to add a parameter to the fields metadata for "vert interp" or something like that, set it to true by default for 3D fields, and then explicitly set it to false in the fields metadata yaml for the fields we know should not be interpolated.

Sounds good @travissluka , the scenario above is exactly what happened ...

@travissluka
Copy link
Contributor

@guillaumevernieres do you know if the remapping is actually tested in any of the ctests? I'm skeptical since none of the answers changed.

@liuxiao37k
Copy link
Contributor

@guillaumevernieres do you know if the remapping is actually tested in any of the ctests? I'm skeptical since none of the answers changed.

I had the same question before, but I was not sure if our ctests ever used a MOM.res.nc on a different vert coordinate. So I just picked a ctest which requires remap_filename: and manually altered the h variable in the file that remap_filename: points to, and re-ran that ctest. The results did change so I just assumed that "it works". But that was probably over a year ago.

@kbhargava
Copy link
Contributor

The build seems fine and the ctests pass on orion, except the usual test_ens_hofx that fails for me everytime on orion.

@guillaumevernieres
Copy link
Contributor Author

@guillaumevernieres do you know if the remapping is actually tested in any of the ctests? I'm skeptical since none of the answers changed.

@travissluka , yes, it's "tested" in a few of the ctests, but we interpolate to the same vertical coordinate ... so ... it's "tested". I did check that it actually cycles through the correct variables.

Copy link
Contributor

@kbhargava kbhargava left a comment

Choose a reason for hiding this comment

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

@guillaumevernieres can you add a log statement so that the check you manually did is reflected in the test logs/results, pretty please?

@guillaumevernieres
Copy link
Contributor Author

@guillaumevernieres can you add a log statement so that the check you manually did is reflected in the test logs/results, pretty please?

log4kriti

Good enough @kbhargava ?

Copy link
Contributor

@travissluka travissluka left a comment

Choose a reason for hiding this comment

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

@travissluka travissluka merged commit 068498a into develop Jul 13, 2023
@travissluka travissluka deleted the feature/remap_uv branch July 13, 2023 15:26
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.

Add u,v to the vertical remapping

4 participants