-
Notifications
You must be signed in to change notification settings - Fork 369
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: add a DataInterpolation class #6812
base: master
Are you sure you want to change the base?
Conversation
|
d6d24a8
to
261933d
Compare
Simplifies some operations that looked clunky on the client end
* The class handles time, horizontal, and vertical interpolation * The vertical/horizontal are optional, but the time dimension is REQUIRED
261933d
to
eb23fd7
Compare
eb23fd7
to
6bfbb0f
Compare
We may not have any mid or any int field. No need to force us to create a valid field just for passing checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to break this plan into three PRs. This one which introduces the DataInterpolation Class and applies it to time interpolation and then subsequent ones with horiz and vertical?
* and allows two things: compute the interval length (in days), and check if | ||
* a timestamp lies within the interval. | ||
* | ||
* When te TimeLine arg to the ctor is YearlyPeriodic, the year part of beg/end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"te" should be "the"
I don't think so. The three interpolations are tightly coupled via remappers. Sure, the remapper can be For reviewers: I try to isolate concepts into their own commits. So one way to make reviews easier is to review one commit at a time. |
The class is in charge of reading in time-dependent datasets from input file(s), interpolate to current model time, and possibly do horizontal/vertical interpolation
The implementation is not yet complete, but some early thoughts on whether we're on the right track or not may help. Besides, getting some early testing on ghci machines can also help.
Things left to do:
I am NOT implementing a data interpolation for variables without a time dependence. It simply reduces to constructing 2 remappers, and since non-time dep data is most likely load-able at init, there's no real need for a complex data structure. Time interpolation is the most tricky of the 3 anyways, due to the different needs (nudging uses Linear timeline, while SPA uses a YearlyPeriodic one)
For reviewers: i recommend reviewing one commit at a time. That would help isolate concepts, so you can hopefully better follow. The hardest thing to review is the data interpolation unit tests, since there is quite a bit of code needed to generate cases that we can manually check without redoing the same operation as the interpolation class (copy+paste of src code in tests folder doesn't help).
Fixes #6810
Fixes #6820