-
Notifications
You must be signed in to change notification settings - Fork 132
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
io: allow disabling coordinates in history files #935
Conversation
This seems fine, will review in more detail soon. I'd like to wait until #928 is merged before merging this as there may be some conflicts. |
Yes, I just tried merging #928 into my branch and there are indeed conflicts. I'll update my branch when #928 is merged. One thing I thought about: removing the coordinates does make the history file non-CF compliant, I think, but they are already non-compliant if The only mention of the CF conventions I found was here: CICE/doc/source/user_guide/ug_implementation.rst Line 1173 in 095e62a
|
Thanks for doing this Phil, it had been somewhere on my to-do list! It saves time when concatenating results in python/xarray - because the default behaviour there is to check all the coordinates are identical for every file opened. Other models write these time-invariant coordinates once during a run (to a seperate file) - we don't have to do that now, but it might be a nice thing to have. I agree its probably not-CF compliant, but I do think its useful behavior. |
Yes, we already have an issue for that: #613. I think it is a separate issue, and being able to disable the coordinates is nice in the mean time. |
Interesting. I thought we had this. We just had f_bounds I guess. Do we really want to customize it in this detail? Can we do an all or none flag? |
I prefer to have more granularity. This can be useful for example for B-grid runs, one might not want the So that's why I went with individual flags. It seems easier than tying it to |
By the way, this PR is best (re)viewed using |
Fair enough. This looks good to me. |
I merged #928, as expected, this now has conflicts. @phil-blain, please update when you have a chance. Thanks. |
ebe9008
to
0ddd23c
Compare
Users might not wish for all 8 coordinate variables to be written to each history file, so add namelist flags allowing to disable each coordinate variable independantly, following the approach used for the other grid variables. Move the definition of 'ncoord' from each IO backend to 'ice_history_shared'. Note that we already 'use' the whole of ice_history_shared in ice_history_write, so we do not need to adjust the use statement. Note that the code that writes these variables in module 'ice_history_write' in each of the IO backends is not modified, it will be adjusted in a following commit.
…a loop In ice_history_write::ice_write_hist, we initialize the 'var_coord' and 'coord_bounds' arrays by manually incrementing 'ind' and initializing each elements with the corresponding information for each coordinate variable. The rest of the code in that subroutine instead uses a loop on 'ncoord', which is a parameter holding the number of coordinates variable, along with 'select' statements. The latter approach is more elegant and also more flexible if we change the number of coordinates variables. Refactor the code to use a loop and leverage the integers n_{u,t,n,e}{lon,lat} added in the previous commit, in both io_netcdf and io_pio2, which have identical code in this part of the subroutine.
… variables In order to enable the namelist flags for each coordinate variables added in the previous commit, adjust the code of the io_netcdf and io_pio2 backends by checking the value of 'icoord(i)' for each loop on 'ncoord' that calls NetCDF or PIO functions. Add the new flags to the reference namelist.
0ddd23c
to
860d163
Compare
I've just rebased the branch and tweaked a little something in 1/3 (which is now 2/3). I've launched a new base suite just in case. |
base_suite is still bit for bit. |
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 tested on Derecho with io_suite including pio and everything seems to be working well.
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
* ice_history_shared: add namelist flags for coordinate variables Users might not wish for all 8 coordinate variables to be written to each history file, so add namelist flags allowing to disable each coordinate variable independantly, following the approach used for the other grid variables. Move the definition of 'ncoord' from each IO backend to 'ice_history_shared'. Note that we already 'use' the whole of ice_history_shared in ice_history_write, so we do not need to adjust the use statement. Note that the code that writes these variables in module 'ice_history_write' in each of the IO backends is not modified, it will be adjusted in a following commit. * io_{netcdf,pio2}/ice_history_write: define coordinates attributes in a loop In ice_history_write::ice_write_hist, we initialize the 'var_coord' and 'coord_bounds' arrays by manually incrementing 'ind' and initializing each elements with the corresponding information for each coordinate variable. The rest of the code in that subroutine instead uses a loop on 'ncoord', which is a parameter holding the number of coordinates variable, along with 'select' statements. The latter approach is more elegant and also more flexible if we change the number of coordinates variables. Refactor the code to use a loop and leverage the integers n_{u,t,n,e}{lon,lat} added in the previous commit, in both io_netcdf and io_pio2, which have identical code in this part of the subroutine. * io_{netcdf,pio2}/ice_history_write: use namelist flags for coordinate variables In order to enable the namelist flags for each coordinate variables added in the previous commit, adjust the code of the io_netcdf and io_pio2 backends by checking the value of 'icoord(i)' for each loop on 'ncoord' that calls NetCDF or PIO functions. Add the new flags to the reference namelist.
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once. See CICE-Consortium/CICE#935
PR checklist
See title.
me.
Base suite is bit for bit (EDIT I did run the io_suite but I do not have PIO on our machines so the PIO tests fail to build).