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

no remap when not necessary #53

Merged
merged 16 commits into from
Feb 21, 2024
Merged

no remap when not necessary #53

merged 16 commits into from
Feb 21, 2024

Conversation

weiyuan-jiang
Copy link
Contributor

This PR address issue #52

@weiyuan-jiang weiyuan-jiang added the Non 0-diff The changes in this pull request are non-zero-diff label Jan 26, 2024
Copy link

This PR is being prevented from merging because you have not added one of our required labels: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Please add one so that the PR can be merged.

@biljanaorescanin
Copy link
Contributor

PR was tested for these cases:
If same atm res, same lev , diff ocean , same land => we will link upper restarts
If same atm res, same lev, same ocean, diff land => we will remap upper restarts
If same atm res, diff lev, same ocean, same land => we will remap upper restarts
Let me know if any other cases need to be tested.
@gmao-rreichle

@weiyuan-jiang
Copy link
Contributor Author

PR was tested for these cases: If same atm res, same lev , diff ocean , same land => we will link upper restarts If same atm res, same lev, same ocean, diff land => we will remap upper restarts If same atm res, diff lev, same ocean, same land => we will remap upper restarts Let me know if any other cases need to be tested. @gmao-rreichle

"diff land" here is diff topo

@gmao-rreichle
Copy link
Contributor

@sdrabenh, @biljanaorescanin, @weiyuan-jiang: It looks like this PR is meant to address a situation when upper air restart files do not need to be remapped and can just be linked. But why not apply the same checks to the land restarts? If the atm grid and bcs version (excl the topo files) are the same, land restarts don't need to be remapped. Any differences in the number of atm levels, the ocean grid, or the topo files shouldn't matter to the land restarts. I don't know how long it takes to remap high-res land restarts, but I imagine some savings could be realized by avoiding the unnecessary remapping of land restarts. Just a thought.

@weiyuan-jiang
Copy link
Contributor Author

@sdrabenh, @biljanaorescanin, @weiyuan-jiang: It looks like this PR is meant to address a situation when upper air restart files do not need to be remapped and can just be linked. But why not apply the same checks to the land restarts? If the atm grid and bcs version (excl the topo files) are the same, land restarts don't need to be remapped. Any differences in the number of atm levels, the ocean grid, or the topo files shouldn't matter to the land restarts. I don't know how long it takes to remap high-res land restarts, but I imagine some savings could be realized by avoiding the unnecessary remapping of land restarts. Just a thought.

If everything is the same, then why do remap ?

@gmao-rreichle
Copy link
Contributor

@weiyuan-jiang:

  • If "topo" and "agrid" and "levels" are the same, we don't need to remap the upper air restarts. The "bcs version" could still be different (in something other than the "topo" files), which would mean that we have to remap the land restarts. I think that's what you implemented in this PR.
  • If "agrid" and "bcs_version" are the same, we don't need to remap the land restarts. The "levels" could still be different, which would mean that we would have to remap the upper air restarts.

These are just two quick example situations. I didn't include the "ogrid" or the stretch factors in the example. The stretch factors should probably be considered in a bundle with "agrid". The full specs of the horizontal grid consist of the resolution ("agrid") and the stretch factors.

There are probably other combinations for which some (groups of) restarts may need to be remapped but not others.

My (admittedly vague) memory is that remapping land restarts can take a long time for high resolution bcs.

@weiyuan-jiang
Copy link
Contributor Author

@weiyuan-jiang:

  • If "topo" and "agrid" and "levels" are the same, we don't need to remap the upper air restarts. The "bcs version" could still be different (in something other than the "topo" files), which would mean that we have to remap the land restarts. I think that's what you implemented in this PR.
  • If "agrid" and "bcs_version" are the same, we don't need to remap the land restarts. The "levels" could still be different, which would mean that we would have to remap the upper air restarts.

These are just two quick example situations. I didn't include the "ogrid" or the stretch factors in the example. The stretch factors should probably be considered in a bundle with "agrid". The full specs of the horizontal grid consist of the resolution ("agrid") and the stretch factors.

There are probably other combinations for which some (groups of) restarts may need to be remapped but not others.

My (admittedly vague) memory is that remapping land restarts can take a long time for high resolution bcs.

Indeed, there are some cases that land restarts don't need to remap. I will make some changes

@weiyuan-jiang weiyuan-jiang changed the title no remap upper air when not necessary no remap when not necessary Jan 29, 2024
@weiyuan-jiang
Copy link
Contributor Author

The latest commit will not do the remapping if it is not necessary for all restarts

@weiyuan-jiang
Copy link
Contributor Author

I changed link to copy. I prefer the output folder has all the restart files.

@biljanaorescanin
Copy link
Contributor

Testing summary:
a) Automated tests passed
b) In red is what is diff to starting restart set
Screenshot 2024-02-02 at 3 09 08 PM
@gmao-rreichle

@gmao-rreichle gmao-rreichle marked this pull request as ready for review February 2, 2024 20:31
@gmao-rreichle gmao-rreichle requested review from a team as code owners February 2, 2024 20:31
gmao-rreichle
gmao-rreichle previously approved these changes Feb 2, 2024
@gmao-rreichle gmao-rreichle added 0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) non-0-diff for remap_restarts.py and removed Non 0-diff The changes in this pull request are non-zero-diff labels Feb 2, 2024
gmao-rreichle
gmao-rreichle previously approved these changes Feb 7, 2024
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.

Re-approving after 0-diff-trivial commit, which only updated USAGE text. No need to re-test.

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.

Approving again after one more 0-diff-trivial commit (making remap_restarts.py usage text consistent with corresponding Wiki page)

@sdrabenh sdrabenh merged commit 4beb105 into main Feb 21, 2024
11 checks passed
@sdrabenh sdrabenh deleted the feature/wjiang/no_remap_incase branch February 21, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) non-0-diff for remap_restarts.py
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants