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

MITgcm/N-BLING with Compressed Staggered Grids #640

Merged
merged 67 commits into from
Mar 12, 2024

Conversation

mgharamti
Copy link
Contributor

@mgharamti mgharamti commented Feb 20, 2024

Description:

The MITgcm code now supports compressed grids, especially suited for areas like the Red Sea where land occupies more than 90% of the domain. Thanks to Ed Liu (SIParCS intern, 2022) and Helen Kershaw for developing this approach.

Fixes issue

The updated code allows writing the bgc fields into MITgcm's pickup files. It also allows for different compression of the regular and staggered grids. These changes have been tested at KAUST for the Arabian Gulf in cycling DA runs.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Documentation changes needed?

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.

Tests

Please describe any tests you ran to verify your changes.

Checklist for merging

  • Updated changelog entry
  • Documentation updated
  • Update conf.py

Checklist for release

  • Merge into main
  • Create release from the main branch with appropriate tag
  • Delete feature-branch

Testing Datasets

  • Dataset needed for testing available upon request
  • Dataset download instructions included
  • No dataset needed

Ed Liu and others added 30 commits June 6, 2022 15:33
todo: use dart modules rather than standalone, netcdf, kinds, etc
mit_to_dart, dart_to_mit seems like place to put the offline
squishing
used when uncompressing
integers for coord index
the caching was fixed in NCAR#368
hkershaw-brown and others added 14 commits October 11, 2022 09:47
CHL is not updated, but may be helpful when testing code.
For example, perturbing uncompressed state to get an ensemble of
mit .data files to test bitwise between compressed and non-compressed.
Changes made to trans_mit_dart:
- trans_mit_dart now uses separate compression dimensions for U and V
- T, SAL and other tracers still use original compression
- Eta's compression is done through the T (or SAL) surface layer
to avoid any zero pressure (height) issues
- The tracers after DART are written into MITgcm's pickup files

Changes to model_mod:
- Getting compressed state index now directly uses comp_inds from the
restart files
Suggested changes to the compressed code after testing
for the Arabian Gulf. The code has been reported to work
fine for several DA cycles. A small bug was fixed when
parsing the vertical layers for the pickup files.
@hkershaw-brown
Copy link
Member

Hi Moha, great work on this and thanks to the KAUST folks for testing it thoroughly. It is a really good improvement on the current code.
One question for you:
There is a program expand_netcdf which was used to uncompress the netcdf file into a non-compressed netcdf file - this was for MITgcm novices (like me) taking a look at the data with ncview while writing the compression code. It does not uncompress for the different grids, and so its not complete at the movement. Are you ok removing this (assuming it is not what regular DA user would need?)

I do want check on the netcdf_utilies_mod 64 bit offset change to see if we need to be careful/paranoid about breaking netcdf writes: reviewers see notes https://github.com/NCAR/DART/pull/455/files#r1101999108, #287

Then a couple of notes for whoever does the release: check with Ed how he wants his name, and here is his AMS presentation and siparcs page to add to the release notes.
From work by Jiachen Liu which he presented at AMS 2023
The work was done as part of SIParCS 2022 under the Red Sea Initiative project

Copy link
Member

@hkershaw-brown hkershaw-brown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting this as 'request changes' purely for the netcdf file_creation 64_bit_offset change. Need to check (& prevent) unintended consequences for this.

Comment on lines 558 to +562
! CLM has (potentially many) columns and needs i7 ish precision
write(string1,'(i11,1x,''i,j,k'',3(1x,i7),'' domain '',i2)') &
! write(string1,'(i11,1x,''i,j,k'',3(1x,i7),'' domain '',i2)') &
! iloc, ix, iy, iz, dom_id
! EL: integer to short for the new I/O method
! Change to long int to avoid problems
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can tidy this comment i21 for long ints.

@mgharamti
Copy link
Contributor Author

Helen, thanks for taking time to review this PR.
To your first question, I have no problem removing the expand_netcdf program. I understand the reason behind it yet I didn't run it myself. Siva, used it a couple of times before extending the compression capability but as you noted it's not needed for operating filter.
Good observation regarding the 64 bit netcdf thing, I am not sure if this causes any issues with other nc files. I guess I'm not experienced enough on this front to give a useful suggestion.
I fully support acknowledging Ed in the best possible way for this release.

hkershaw-brown and others added 2 commits March 11, 2024 09:00
see pull NCAR#640 for discussion
* does not have the u, v compression
@hkershaw-brown hkershaw-brown added the release! bundle with next release label Mar 11, 2024
Copy link
Member

@hkershaw-brown hkershaw-brown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved!

@hkershaw-brown add Ed to release notes, edit:
the smaller pull requests are getting bundled with the wrf-tutorial fixes #636 #651 #637

@hkershaw-brown hkershaw-brown merged commit cd7189e into NCAR:main Mar 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release! bundle with next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants