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

Checkpointer validates on C_SW and D_SW #320

Merged
merged 21 commits into from
Sep 20, 2022
Merged

Conversation

elynnwu
Copy link
Collaborator

@elynnwu elynnwu commented Sep 9, 2022

Purpose

In this PR, checkpointers are passing at FVDynamics-In, C_SW-In/Out, D_SW-In/Out, and failing on qvapor with small error in FVDynamics-Out (passing on all other variables) with the automatically calibrated thresholds. Further investigation is needed.

Additionally, in Fortran serialized data, FVDynamics-Out has filled values in the halos for tracers. Instead of subsetting them, we do not serialize them out in the latest 8.1.2 dataset.

Code changes:

  • dyn_core: fixed a mistake in the variables being passed to FVDynamics-In checkpointer
  • translate_fvdynamics/driver: we do not subset tracers since they are cropped in the serialized data
  • test_translate: skip subsetting tracers when True

Checklist

  • You have followed the coding standards guidelines established at Code Review Checklist.
  • Docstrings and type hints are added to new and updated routines, as appropriate
  • All relevant documentation has been updated or added (e.g. README, CONTRIBUTING docs)
  • For each public change and fix in pace-util, HISTORY has been updated
  • Unit tests are added or updated for non-stencil code changes

@@ -691,7 +691,7 @@ def _checkpoint_dsw_in(self, state: DycoreState):
ucd=state.uc,
vcd=state.vc,
wd=state.w,
delpcd=self.delpc,
delpcd=self._vt,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is wild to me that delpc is still a temporary but it's not the variable in the savepoint.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add some # commenting about this? It's very confusing.

@elynnwu elynnwu marked this pull request as ready for review September 12, 2022 17:08
Copy link
Collaborator

@mcgibbon mcgibbon left a comment

Choose a reason for hiding this comment

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

We should be able to avoid changing test_translate.py, after that I think we can merge.

@@ -691,7 +691,7 @@ def _checkpoint_dsw_in(self, state: DycoreState):
ucd=state.uc,
vcd=state.vc,
wd=state.w,
delpcd=self.delpc,
delpcd=self._vt,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add some # commenting about this? It's very confusing.

@elynnwu elynnwu changed the title Fix/checkpointer validation Checkpointer validates on C-SW and D-SW Sep 12, 2022
@elynnwu elynnwu changed the title Checkpointer validates on C-SW and D-SW Checkpointer validates on C_SW and D_SW Sep 12, 2022
@elynnwu
Copy link
Collaborator Author

elynnwu commented Sep 13, 2022

launch jenkins

Copy link
Collaborator

@mcgibbon mcgibbon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@elynnwu
Copy link
Collaborator Author

elynnwu commented Sep 15, 2022

launch jenkins

2 similar comments
@elynnwu
Copy link
Collaborator Author

elynnwu commented Sep 15, 2022

launch jenkins

@elynnwu
Copy link
Collaborator Author

elynnwu commented Sep 19, 2022

launch jenkins

@mcgibbon
Copy link
Collaborator

I approve of merging without the driver_savepoints_mpi-gt:cpu_ifirst plan, which is currently having some issues and might need a larger resource type.

@elynnwu elynnwu merged commit 770da80 into main Sep 20, 2022
@elynnwu elynnwu deleted the fix/checkpointer-validation branch September 20, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants