-
Notifications
You must be signed in to change notification settings - Fork 39
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
Update to 1.7.0 alpha.2 #881
Conversation
This needs: |
62fd06a
to
fe92eb2
Compare
c27fa8a
to
c30a74c
Compare
TestingI ran both the |
@trhille and @matthewhoffman, I will need your help to make sure I didn't break anything for you 2. Can you run tests that go beyond |
@@ -688,22 +688,22 @@ def build_mali_mesh(self, cell_width, x1, y1, geom_points, | |||
logger.info('writing grid_converted.nc') | |||
write_netcdf(dsMesh, 'grid_converted.nc') | |||
levels = section.get('levels') | |||
args = ['create_landice_grid_from_generic_MPAS_grid.py', | |||
args = ['create_landice_grid_from_generic_mpas_grid', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have to remind myself of the details. You should not try to call the entry-point functions because the command-line arguments are still expected for those and they won't work as expected. But in many cases there are other functions with "proper" argument you could call instead and that might be preferable.
In any case, yes, I tried to change things minimally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this further:
https://github.com/MPAS-Dev/MPAS-Tools/blob/701c8d97dfa733ec7128f32aff5ddd4f48a14e2c/conda_package/mpas_tools/landice/create.py#L10
The function create_from_generic_mpas_grid()
as written is not really callable because it assumes that OptionParser.parse_args()
will return the expected command-line arguments (which compass
obviously wouldn't do). Elsewhere in mpas_tools
, we have a function with proper arguments that can be called directly and then we have entry point function that do arg parsing and then call the other function. It would be great to move in that direction but I didn't try to do that for you all because it was one step too far for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xylar , got it - that makes sense. I'm happy with changing things in this way for now (here and the companion MPAS-Tools PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved based on the testing described here: MPAS-Dev/MPAS-Tools#596
2ff5ff2
to
fa44af4
Compare
The NetCDF4 helper function from pyremap is no longer available, and pyremap has switched to this xarray syntax.
We want to make sure they're removed, but it doesn't matter if they weren't installed in the first place.
The environment name needs to be set. The test has also been given a better name.
fa44af4
to
044ace2
Compare
@matthewhoffman and @trhille, the versions of pyremap and mpas_tools that this update depends on have been released. There is a non-zero chance that I've missed a few updates needed for either of these. We can either do more testing now, before this goes in or we can go ahead and merge this, and fix anything that remains in follow-up PRs. Basically, it's a question of how much you want to suffer now vs. potentially suffer later... |
I'll certainly re-test the |
Further testing
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving based on inspection and @trhille's testing
Thanks @trhille and @matthewhoffman!! |
This brings in:
As a result of the
mpas_tools
update, many command-line tools used bylandice
have been renamed. The relevant documentation has also been updated.Checklist
Testing
in this PR) any testing that was used to verify the changes