Skip to content
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

Merged
merged 3 commits into from
Feb 23, 2024

Conversation

phil-blain
Copy link
Member

@phil-blain phil-blain commented Feb 19, 2024

PR checklist

  • Short (1 sentence) summary of your PR:
    See title.
  • Developer(s):
    me.
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.
    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).
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on Icepack or any other models?
    • Yes
    • No
  • Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please document the changes in detail, including why the changes are made. This will become part of the PR commit log.
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.

@phil-blain phil-blain marked this pull request as draft February 19, 2024 17:14
@phil-blain phil-blain marked this pull request as ready for review February 19, 2024 19:02
@apcraig
Copy link
Contributor

apcraig commented Feb 19, 2024

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.

@phil-blain
Copy link
Member Author

phil-blain commented Feb 19, 2024

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 f_bounds is .false., and this is the default in our namelist (though the code defaults it to .true.). So I think this is OK, but, should we add a note to the doc somewhere ? and mention f_bounds while at it...

The only mention of the CF conventions I found was here:

``ICE_IOTYPE``). The netCDF history files are CF-compliant; header information for

@anton-seaice
Copy link
Contributor

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.

@phil-blain
Copy link
Member Author

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.

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.

@dabail10
Copy link
Contributor

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?

@phil-blain
Copy link
Member Author

phil-blain commented Feb 22, 2024

I prefer to have more granularity. This can be useful for example for B-grid runs, one might not want the E and N lat/lon but might want to keep the T/U lat/lon.

So that's why I went with individual flags. It seems easier than tying it to grid_ice, and lets the user choose what they want.

@phil-blain
Copy link
Member Author

By the way, this PR is best (re)viewed using git log --color-moved --color-moved-ws=allow-indentation-changes, which the GitHub "Files changed" tabs does not use.

@dabail10
Copy link
Contributor

Fair enough. This looks good to me.

@apcraig
Copy link
Contributor

apcraig commented Feb 22, 2024

I merged #928, as expected, this now has conflicts. @phil-blain, please update when you have a chance. Thanks.

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.
@phil-blain
Copy link
Member Author

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.

@phil-blain
Copy link
Member Author

base_suite is still bit for bit.

Copy link
Contributor

@apcraig apcraig left a 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.

@apcraig apcraig merged commit 64177e3 into CICE-Consortium:main Feb 23, 2024
2 checks passed
@phil-blain phil-blain deleted the disable-coords branch February 23, 2024 13:07
anton-seaice added a commit to ACCESS-NRI/access-om3-configs that referenced this pull request Apr 5, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
DeniseWorthen pushed a commit to DeniseWorthen/CICE that referenced this pull request Apr 13, 2024
* 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.
anton-seaice added a commit to ACCESS-NRI/access-om3-configs that referenced this pull request May 14, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
anton-seaice added a commit to ACCESS-NRI/access-om3-configs that referenced this pull request May 16, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
anton-seaice added a commit to ACCESS-NRI/access-om3-configs that referenced this pull request May 16, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
anton-seaice added a commit to ACCESS-NRI/access-om3-configs that referenced this pull request May 16, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
anton-seaice added a commit to ACCESS-NRI/access-om3-configs that referenced this pull request May 17, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
anton-seaice added a commit to ACCESS-NRI/access-om3-configs that referenced this pull request May 17, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
anton-seaice added a commit to ACCESS-NRI/access-om3-wav-configs that referenced this pull request May 17, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
anton-seaice added a commit to ACCESS-NRI/access-om3-wav-configs that referenced this pull request May 17, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
anton-seaice added a commit to ACCESS-NRI/access-om3-wav-configs that referenced this pull request May 17, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
anton-seaice added a commit to ACCESS-NRI/access-om3-wav-configs that referenced this pull request May 17, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
anton-seaice added a commit to ACCESS-NRI/access-om3-wav-configs that referenced this pull request May 17, 2024
…ave a land(ish) mask applied, so are not very useful and slow down multiple history file at once.

See CICE-Consortium/CICE#935
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants