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

EAMxx: make SPA use DataInterpolation #6920

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

bartgol
Copy link
Contributor

@bartgol bartgol commented Jan 21, 2025

Allow SPA to use the DataInterpolation class. Changes include:

  • accommodate IOP horizontal remapping
  • allow to set reference timestamp from API rather than read from file
  • replace SPAFunctions with DataInterpolation
  • remove unit test in spa folder

[Non BFB]


This PR will be rebased on master once #6914 and #6919 are merged.

@bartgol bartgol added EAMxx PRs focused on capabilities for EAMxx code cleanup labels Jan 21, 2025
@bartgol bartgol self-assigned this Jan 21, 2025
@bartgol bartgol force-pushed the bartgol/eamxx/spa-use-data-interpolation branch 3 times, most recently from 87adc7b to 0bb87be Compare January 21, 2025 17:18
Copy link
Contributor

@AaronDonahue AaronDonahue left a comment

Choose a reason for hiding this comment

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

Just a couple spelling mistakes that I found. Otherwise it looks good. Its great to see that we can remove so many lines of code and define SPA in a much simpler framework.

Given that remap setup options started to increase too much,
I added a RemapData mini-struct, that customers need to pass
Allow to pass in the reference time stamp rather than
reading it in from the input files
Also remove the unit tests, since SPA is simply a wrapper
of a DataInterpolation instance
@bartgol bartgol force-pushed the bartgol/eamxx/spa-use-data-interpolation branch from 0bb87be to 2244552 Compare January 29, 2025 23:12
@bartgol bartgol requested a review from AaronDonahue January 29, 2025 23:12
@bartgol
Copy link
Contributor Author

bartgol commented Jan 31, 2025

All spa tests are diffing. I was not expecting that, so I need to investigate further.

@bartgol
Copy link
Contributor Author

bartgol commented Jan 31, 2025

Most fails are due to how SPA data is used. Current master consider spa data for month X to be at time "beg of month X". Since the SPA file shows the middle of the month as the "time" var (which is logical, for monthly averaged data), DataInterpolation does a different interpolation than master. Changing the time var to store the time corresponding to beginning of the month for each month makes all diffing tests pass cmp baselines. Since we agreed in a slack convo that the behavior of the current branch is less biased, we're going to ignore those fails. The reasoning is that, say, Jan02 is closer to Dec than Feb, so the Dec avg should be used rather than the Feb one. Master interpolates current_month and next_month, while this branch interpolates current_month and "closest_month" (which could be next or prev).

There is one runtime fail though, when running dp. I am investigating.

@bartgol bartgol requested a review from AaronDonahue January 31, 2025 21:43
@bartgol
Copy link
Contributor Author

bartgol commented Jan 31, 2025

All standalone tests pass if I modify the spa file to point to the beg of the month for each month (rather than middle of the month), so that DataInterpolation does the same as the old SPA functions. I am now testing a v1 case, to make sure that's the case also in v1. If so, we can merge and re-bless all eamxx baselines.

@bartgol
Copy link
Contributor Author

bartgol commented Feb 7, 2025

I ran one of our v1 nightly tests on mappy with this PR. With the current spa file, it diffs (as expected), but with a "doctored" spa file (where each month time coord is at 00:00 of the 1st), the outputs are identical.

@AaronDonahue I feel confident that the diffs are just due to how SPA slices were declared in the input file. If you don't have any more comment, I will merge (maybe on Monday, since it will require bless requests).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup EAMxx PRs focused on capabilities for EAMxx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants