Skip to content

use nc4 tile file when remapping restarts #1094

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.

Sign up for GitHub

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

Jump to bottom
Merged
sdrabenh merged 5 commits into from
May 15, 2025
Merged

use nc4 tile file when remapping restarts #1094

sdrabenh merged 5 commits into from
May 15, 2025

Conversation

@weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented Apr 24, 2025 •
edited by gmao-rreichle
Loading

If a tile file is nc4, the re-tiling subroutine should be able to read it.

Some cleanup of mk_restarts.

Required by: GEOS-ESM/GEOSldas#705

Trivially 0-diff for GCM and LDAS simulations. 0-diff tests ok for remap_restarts: GEOS-ESM/GEOS_Util#137 (comment)

@weiyuan-jiang weiyuan-jiang added bug Something isn't working 0 diff The changes in this pull request have verified to be zero-diff with the target branch. labels Apr 24, 2025
@weiyuan-jiang weiyuan-jiang requested a review from a team as a code owner April 24, 2025 13:45
@weiyuan-jiang weiyuan-jiang requested a review from a team as a code owner April 24, 2025 16:49
@weiyuan-jiang weiyuan-jiang mentioned this pull request Apr 25, 2025
Merged
@gmao-rreichle gmao-rreichle mentioned this pull request Apr 30, 2025
Merged
@biljanaorescanin
Copy link
Contributor

biljanaorescanin commented May 2, 2025

Testing summary:

  1. GEOSldas tests passed
  2. GCM 1-day amip, replay and inc replay all passed

This PR has a bug fix but not visible for GCM since we still don't use NC4 tile file there.

@gmao-rreichle gmao-rreichle added bugfix This fixes a bug and removed bug Something isn't working labels May 2, 2025
gmao-rreichle
gmao-rreichle previously approved these changes May 2, 2025
View reviewed changes
@weiyuan-jiang
Copy link
Contributor Author

weiyuan-jiang commented May 2, 2025 •
edited by gmao-rreichle
Loading

Testing summary:

  1. GEOSldas tests passed
  2. GCM 1-day amip, replay and inc replay all passed

This PR has a bug fix but not visible for GCM since we still don't use NC4 tile file there.

Can you be more specific about this statement "This PR has a bug fix but not visible for GCM since we still don't use NC4 tile file there" ? Thanks @biljanaorescanin

gmao-rreichle
gmao-rreichle reviewed May 2, 2025
View reviewed changes
Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

Revoking approval for now... Some issues came up with testing remap_restarts. @biljanaorescanin is working on it

@gmao-rreichle gmao-rreichle dismissed their stale review May 2, 2025 19:33

Revoking approval for now... Some issues came up with testing remap_restarts. @biljanaorescanin is working on it

@gmao-rreichle gmao-rreichle marked this pull request as draft May 2, 2025 19:33
@biljanaorescanin
Copy link
Contributor

biljanaorescanin commented May 2, 2025 •
edited
Loading

Can you be more specific about this statement "This PR has a bug fix but not visible for GCM since we still don't use NC4 tile file there" ?

@weiyuan-jiang Sorry, I taught this was other PR with fix during population of nc4 file for elevation. This one just uses nc4 instead of ascii if available. So why do we have bug/bug fix label? It is still zero diff PR as far as testing.

@gmao-rreichle
Copy link
Contributor

gmao-rreichle commented May 5, 2025 •
edited
Loading

Can you be more specific about this statement "This PR has a bug fix but not visible for GCM since we still don't use NC4 tile file there" ?

@weiyuan-jiang Sorry, I taught this was other PR with fix during population of nc4 file for elevation. This one just uses nc4 instead of ascii if available. So why do we have bug/bug fix label? It is still zero diff PR as far as testing.

FYI, I had replaced the "bug" label with the "bug fix" label, but I didn't question whether the "bug" label is appropriate. I agree with Biljana that it's unclear why the "bug" label was applied originally. If there was a bug that is now fixed, we need to clarify what was wrong / fixed. Otherwise, we should remove the "bug fix" label.

@gmao-rreichle gmao-rreichle changed the title use nc4 tilefile when re-tiling use nc4 tilefile when remapping restarts May 6, 2025
@gmao-rreichle gmao-rreichle changed the title use nc4 tilefile when remapping restarts use nc4 tile file when remapping restarts May 6, 2025
@biljanaorescanin biljanaorescanin removed the bugfix This fixes a bug label May 7, 2025
@gmao-rreichle gmao-rreichle marked this pull request as ready for review May 8, 2025 14:04
gmao-rreichle
gmao-rreichle previously approved these changes May 8, 2025
View reviewed changes
gmao-rreichle
gmao-rreichle approved these changes May 8, 2025
View reviewed changes
Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

@sdrabenh: This PR is ready for merging. We will also need a new release of GEOSgcm_GridComp. (This is needed to create a release of GEOSldas that is capable of running landice tiles.) Please let me know if you have any questions, thanks!

@sdrabenh sdrabenh merged commit 2f5654d into develop May 15, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sdrabenh sdrabenh sdrabenh approved these changes

@gmao-rreichle gmao-rreichle gmao-rreichle approved these changes

Assignees

No one assigned

Labels

0 diff The changes in this pull request have verified to be zero-diff with the target branch. cleanup

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@weiyuan-jiang @biljanaorescanin @gmao-rreichle @sdrabenh