NOC-MSM / pyBDY

pyBDY: a Python based regional NEMO model configuration toolbox.
GNU General Public License v3.0
7 stars 7 forks source link

initial commit for new branch for bypassing vertical interpolation #163

Open rdPatmore opened 4 months ago

rdPatmore commented 4 months ago

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.

jdha commented 4 months ago

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.

jdha commented 4 months ago

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 ...

rdPatmore commented 4 months ago

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?

jdha commented 4 months ago

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 set_src_z_grid_3d to allow a generic zt and if zw doesn't exist create using a crude approximation of mid-points between zt ... that way we won't have destroyed any previous capability of reading in non-NEMO source data.

jdha commented 4 months ago

@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)

rdPatmore commented 4 months ago

Yep, no problem, go ahead and bring it up to date.

rdPatmore commented 4 months ago

@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.

rdPatmore commented 3 months ago

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.

jdha commented 3 months ago

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.

rdPatmore commented 3 months ago

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.