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

Interpolator mask #97

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Interpolator mask #97

wants to merge 1 commit into from

Conversation

s-good
Copy link
Collaborator

@s-good s-good commented Jun 13, 2024

Description

The interpolator currently does not use the mask variable so it returns interpolated values when it shouldn't. In the case of nearest neighbour time interpolation, this results in the model value from the last time point always being returned.

Issue(s) addressed

Resolves #95

Dependencies

None.

Impact

Only affects JOPA runs where the model background has multiple time steps. The configuration files have recently been updated to all use linear interpolation so there will be no effect from this change.

Checklist

  • I have updated the unit tests to cover the change
  • New functions are documented briefly via Doxygen comments in the code
  • I have linted my code using cpplint
  • I have run the unit tests
  • I have run mo-bundle to check integration with the rest of JEDI and run the unit tests under all environments

@s-good s-good requested a review from twsearle June 13, 2024 17:32
@s-good
Copy link
Collaborator Author

s-good commented Jun 13, 2024

I've tested the code on MetOp SST data using the sith suite and it appears to work. However, I'm not 100% confident I understand the way the mask variable is supposed to be used. @twsearle, please could you give an initial review and if you think it looks OK I will work on updating the unit test reference data.

@twsearle
Copy link
Collaborator

This looks correct to me - its slightly different from how others implement it in that we ensure the default value is always the missing data indicator, however I still think this lines up with the behaviour of the generic atlas interpolator here:
https://github.com/JCSDA-internal/oops/blob/c531e3fde8a313349f7a27f3da00f8719858ad00/src/oops/generic/AtlasInterpolator.cc#L53

@twsearle twsearle requested a review from odlomax June 19, 2024 08:32
@twsearle
Copy link
Collaborator

@odlomax Can you confirm this is the correct approach to implementing the oops interpolator "time mask" variable?

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.

Unexpected behaviour when using nearest neighbour time stepping
2 participants