-
Notifications
You must be signed in to change notification settings - Fork 10
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
Bnb/dual dh patch #164
Bnb/dual dh patch #164
Conversation
…this broke the regridding bc lat_lon wasn't being updated after hr_spatial_coarsening.
852cb65
to
4f4e76a
Compare
4f4e76a
to
0cf4b41
Compare
{feature} is one of the features contained by this DataHandler and | ||
the data is a 3D array of shape (lat, lon, time) where time is | ||
length 1 for annual correction or 12 for monthly correction. Bias | ||
correction is only run if bc_files is not None. |
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.
This handler has to be init on independent DataHandler objects, each of which has a bias correction method. Why should we add more logic in this dual handler instead of just bias correcting the LR data handler before being passed in here? Bias correction code should ideally exist in only as many places as is necessary, i would advocate for removing here.
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.
I did this because the lr_handler is expected to load the full ERA domain before regridding (to in many cases a much smaller domain) so bias correcting the regridded data can in theory require touching far fewer points. But maybe it's still not worth duplicating code?
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.
The code that makes the bias correction factors will operate on the original ERA domain though, won't it? Bias correction is really fast. Strong preference for keeping bc functions only in the base handler.
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.
Yes it will but the original ERA domain is not the same as the final domain after regridding to the wtk grid. I load the entire ERA domain in the lr_handler before regridding because its hard to select an ERA subdomain that still covers the wtk domain (due to the curvilinear grid). But if theres no concern about speed I'm fine with removing this.
…lat_lon to use min(dist) instead of kdtree. kept as static method since it is called by class methods.
…dler before sending through dual_data_handler.
No description provided.