Skip to content

Conversation

@bartgol
Copy link
Contributor

@bartgol bartgol commented Jan 17, 2025

Implement an IOP remapper, with the goal of simplifying downstream code, expressing it in terms of more generic interfaces.


@tcclevenger @AaronDonahue You guys should review only the last 3 commits of this branch, since the other ones are from the remappers-api-cleanup branch.

he main part of the PR is the IOPRemapper, but I wanted to verify its correctness by also using it somewhere. The easiest place was SPA, since it has minimal IOP-related stuff (only the single-col remap stuff). All in all, I think SPA gets simplified, and it gets to a good state to be later refactored in terms of a DataInterpolation structure.

I could also start using IOPRemapper in other places of the code (most notably inside the IOPDataManager class itself). Perhaps it's best to do things in steps though, and integrate one small change at a time? Or maybe @tcclevenger could do that step later, since he's more familiar with the IOP class and its assumptions?

Depends on #6908

@bartgol bartgol added BFB PR leaves answers BFB EAMxx C++ based E3SM atmosphere model (aka SCREAM) code cleanup labels Jan 17, 2025
@bartgol bartgol self-assigned this Jan 17, 2025
@bartgol bartgol force-pushed the bartgol/eamxx/iop-remapper branch from 638c9ee to 2dad305 Compare January 20, 2025 17:25
std::random_device rdev;
const int catchRngSeed = Catch::rngSeed();
int seed = catchRngSeed==0 ? rdev() : catchRngSeed;
int seed = catchRngSeed==0 ? rdev()/2 : catchRngSeed;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is b/c you can't do ./my_test --rng-seed -123, since the cmd line arg parser interprets -123 as a flag, not as the value for --rng-seed. Since rdev() is an unsigned int, assigning it to an int can overflow. But if we divide by 2, we are SURE it will be <=2^31-1, which is in the range of int, guaranteeing that seed>=0.

@bartgol bartgol force-pushed the bartgol/eamxx/iop-remapper branch from e6059a5 to db503a5 Compare January 20, 2025 19:49
@bartgol bartgol changed the title EAMxx: add IOPRemapper class and try to use it in SPA EAMxx: add IOPRemapper and use it in SPA and IOPDataManager Jan 20, 2025
@bartgol bartgol force-pushed the bartgol/eamxx/iop-remapper branch from 7282da6 to ea30f24 Compare January 20, 2025 22:35
@tcclevenger
Copy link
Contributor

does this need a rebase now that #6908 is merged?

@bartgol bartgol force-pushed the bartgol/eamxx/iop-remapper branch 2 times, most recently from 5ac2a9f to 6ef410e Compare January 22, 2025 21:36
@bartgol bartgol marked this pull request as draft January 23, 2025 20:17
@bartgol bartgol marked this pull request as ready for review January 23, 2025 20:17
@bartgol bartgol force-pushed the bartgol/eamxx/iop-remapper branch from 6ef410e to f01ebc6 Compare January 23, 2025 23:25
@bartgol
Copy link
Contributor Author

bartgol commented Jan 24, 2025

The CI runs keep getting stuck, and I have to kill them. There's probably a test that hangs. My guess is io_scm_reader. If I can confirm that, I may just remove that class (or at least not compile the src and run the tests), since it was meant for ease of use of pyscream, but I don't think anyone is using pyscream anyways...

@bartgol bartgol force-pushed the bartgol/eamxx/iop-remapper branch from b8d85ef to bafccd6 Compare January 27, 2025 22:05
@bartgol bartgol force-pushed the bartgol/eamxx/iop-remapper branch from bafccd6 to fab7bba Compare January 27, 2025 22:05
@bartgol bartgol force-pushed the bartgol/eamxx/iop-remapper branch from fab7bba to a3925a8 Compare January 27, 2025 22:12
@bartgol
Copy link
Contributor Author

bartgol commented Jan 27, 2025

@AaronDonahue @tcclevenger the tests finally pass and don't hange (I had introduced a tiny bug in SCMInput). You can review whenever you want.

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.

Am I correct in understanding that now SPA uses IOP functions for remapping? So this would make IOP a set of general use functions that could also be used for full model simulations?

@bartgol
Copy link
Contributor Author

bartgol commented Jan 29, 2025

Am I correct in understanding that now SPA uses IOP functions for remapping? So this would make IOP a set of general use functions that could also be used for full model simulations?

Uhm, no. SPA uses IOPRemapper, which does not really know anything about IOP, or IOPDataManager. It only knows about a target lat/lon coordinates. It remaps (ncols_src,nlevs) to (ncols_tgt,nlevs), where the target fields have the same value for all columns, and those value match the column in the source field that is the closest to that lat/lon (across all ranks).

As such, SPA does not know anything about IOP. It only knows that if IOPDataManager is set (it is set in all atm procs by the driver), it has to create an IOPRemapper, using the lat/lon stored inside it.

Copy link
Contributor

@tcclevenger tcclevenger left a comment

Choose a reason for hiding this comment

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

This looks great. I commented on the early return in the case of 0 fields, but feel free to ignore since it's not hurting anything.

@bartgol bartgol merged commit 3ff5aa8 into master Jan 29, 2025
18 of 19 checks passed
@bartgol bartgol deleted the bartgol/eamxx/iop-remapper branch January 29, 2025 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BFB PR leaves answers BFB code cleanup EAMxx C++ based E3SM atmosphere model (aka SCREAM)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants