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

driver can run from serialized init data again and is tested #96

Merged
merged 1 commit into from
Jan 13, 2022

Conversation

rheacangeo
Copy link
Contributor

@rheacangeo rheacangeo commented Jan 12, 2022

Purpose

With the Driver we added the option to initialize from serialized data, because there are experiments we want to run that don't start from the baroclinic analytic state init. We didn't add a test for this functionality, however, and a recent change broke it. This fixes it and adds a driver test that just checks run.py can run, does not do regressions yet (coming soon)

Code changes:

  • update method in the fv3core/testing/translate_fvdynamics code that makes a DycoreState from serialized data
  • update MAX_SIZE for future stencil to accommodate dycore+physics+driver stencils
  • synchronization of the state before writing out is fixed to synchronize the storages now that the state fields are storages

Infrastructure changes:

  • added test_serialized_init to the driver Makefile that checks the driver/run.py script does not crash when starting from serialized data

Checklist

Before submitting this PR, please make sure:

  • 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

…point in the driver Makefile so it is easy to ensure it still works.
@rheacangeo rheacangeo requested a review from elynnwu January 12, 2022 23:44
Copy link
Collaborator

@elynnwu elynnwu left a comment

Choose a reason for hiding this comment

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

lgtm!

@@ -50,7 +50,7 @@ class StencilTable(object, metaclass=Singleton):

DONE_STATE: int = -1
NONE_STATE: int = -2
MAX_SIZE: int = 200
MAX_SIZE: int = 220
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only 20 more, physics is so simple! Will try to remember this as we add more stencils in physics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very true! the actual size is 2*MAX_SIZE + 1, so increased from 401 to 441. Yeah definitely, we can change this number as/when we need, and also the distributed compilation code may end up changing and not using this

@rheacangeo rheacangeo merged commit ee65df5 into main Jan 13, 2022
@rheacangeo rheacangeo deleted the bugfix/driver-init-serialization branch January 13, 2022 18:27
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