initial commit for new branch for bypassing vertical interpolation #163
Conversation
|
FYI - I'm about to commit changes to solve the issue of the pre-commit failing - so you'll have to pull them into this branch. |
|
The one issue with your addition is that the source vertical coordinates are associated with NEMO (>=4.0) domain_cfg files. This should really be generic and handle cases from other models that only have zt ... |
|
Ok, no problem. I agree with your comment about making it generic. For now I was keeping the functionallity as close as possible to the original code so that I didn't introduce too many changes at once. There is quite a significant style change here alongside the interpolation bypass. Perhaps we could merge this in and build on a generic handleing in a follow-up development? |
|
The zgr code was hardcoded as the destination is always NEMO. Although it's outdated now as mbathy no longer exists. I think we can make use of the NCML reader as has been the case previously ... this should be a one liner and will allow the user to redefine the input variable name for gdept_1d/zt in an ncml file should it be different to the NEMO naming convention. Let me push the pyqt6 and pre-commit fixes first ... then I'll take a quick look at the stale PR and see whether we can just drop an option into |
|
@rdPatmore is it ok if I bring this branch in line with the master? it appears to be 30+ commits behind (although these are mainly tide bits) |
|
Yep, no problem, go ahead and bring it up to date. |
bringing branch in line with master
|
@jdha The interpolation bypass requires the use of vertical scale factors when you implement the boundary conditions. I've added these to the processing as there was no functionallity for additional u,v variable before this. There are various discrepanices between my branch and this one so I'll have to reconcile some of that before I push the changes. |
… into 159-keep-original-z-coords updating local branch
|
When using on-the-fly interpolation you are required to supply the vertical scale factors. pybdy had no way of handeling additional variables, so these latest commits address this. You can now add e3t/u/v and the addition is controlled by ln_interp. It stikes me that the way variables are managed is quite convoluted, particularly the u/v pairs. I wonder whether this could be streamlined. The code is still not ready to commit because the interpolation bypass still seems to cut off the bottom levels. I am yet to understand the cause of this error. |
|
I'm a little puzzled by what you've done with the logic for the key_vec option - seemed to be working fine before. I'll have another look this afternoon, but maybe if you can walk me through the changes it might be helpful. Should be free most of tomorrow afternoon. |
|
The original code seemed to only handle one pair of u/v variables: vomecrtx and vozocrty. If you added more to the list, it would ignore them. I've tried to enable the code to handle more variables in order to accomodate e3u and e3v. I did this by making the the loops extend beyond {0,1}. Happy to chat tomorrow afternoon. |
| if ln_dyn2d or ln_dyn3d: | ||
| var_in["u"].extend(["vozocrtx", "vomecrty"]) | ||
| var_in["v"].extend(["vozocrtx", "vomecrty"]) | ||
|
|
||
| if settings["interp"]: # on-the-fly interp requires e3 |
There was a problem hiding this comment.
I hadn't intended using the extract class to deal with grid data. Here, you're passing vertical scale factors as vector objects that will ultimately undergo a rotation! We. can discuss this later - but thought I'd make a note here in the mean time.
There was a problem hiding this comment.
Right, no problem. I was viewing .extend() as an "add variable" function. I'd not thought about concequences of rotation. The scale factors are often output as diagnostics in the same files as the velocities and we need to keep the e3u/e3v components with the velocities when supplying them as boundary conditions. It would be good if .extend() was more of a .add_variable() function, with an option to bypass rotation. That's its function for the grid_T files, right?
There was a problem hiding this comment.
you can view .extend() as an add variable 'function' ... it's just a list method to append variables that are then passed to the extract class. The vertical grid information should really be handled in the coords object, which is how it's set up in master. We haven't extended things to a 3D source depth yet (which will complicate things further) + there's no need to extract the scale factors over time as most of the time they are in some sort of time invariant grid/domain file. If you really want to account for a time variant coordinate then this can be achieved using the ssh.
This is a draft pull request for bypassing vertical interpolation. There is a one line code change that is yet to be added which follows the method of Anna Katavouta.
The refactoring carried out here could help with coordinate issue #140.