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

Update to 1.7.0 alpha.2 #881

Merged
merged 10 commits into from
Apr 9, 2025
Merged

Update to 1.7.0 alpha.2 #881

merged 10 commits into from
Apr 9, 2025

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Jan 9, 2025

This brings in:

  • mpas_tools v1.0.0 -- modernizes command-line tools (including renaming them)
  • pyremap v2.0.0 -- modernization, refactoring, API-breaking changes, MOAB support

As a result of the mpas_tools update, many command-line tools used by landice have been renamed. The relevant documentation has also been updated.

Checklist

  • User's Guide has been updated
  • Developer's Guide has been updated
  • Documentation has been built locally and changes look as expected
  • Document (in a comment titled Testing in this PR) any testing that was used to verify the changes

@xylar xylar added documentation Improvements or additions to documentation land ice clean-up dependencies and deployment Changes relate to creating conda and Spack environments, and creating a load script labels Jan 9, 2025
@xylar xylar self-assigned this Jan 9, 2025
@xylar
Copy link
Collaborator Author

xylar commented Jan 9, 2025

This needs:
MPAS-Dev/MPAS-Tools#596
MPAS-Dev/MPAS-Tools#597
and a new release of MPAS-Tools (v1.0.0) before it will be useful.

@xylar xylar force-pushed the update-to-1.7.0-alpha.2 branch from 62fd06a to fe92eb2 Compare January 11, 2025 15:03
@xylar xylar force-pushed the update-to-1.7.0-alpha.2 branch from c27fa8a to c30a74c Compare January 29, 2025 10:38
@xylar
Copy link
Collaborator Author

xylar commented Jan 29, 2025

Testing

I ran both the pr and full_integration suites on Chrysalis with this new version of mpas_tools and the associated changes in this branch. I didn't check that thing were BFB however and I need to do that.

@xylar
Copy link
Collaborator Author

xylar commented Jan 29, 2025

@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 full_integration to make sure other affected tests aren't broken? Then, can you review this and MPAS-Dev/MPAS-Tools#596 together?

@@ -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',
Copy link
Member

Choose a reason for hiding this comment

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

@xylar , to clarify, we could replace all of these changes with function calls, right? I'm assuming you retained the check_call calls to minimize how invasive this PR is. @trhille and I are on board with that, but just wanted to make sure we understood what's going on.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Member

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).

Copy link
Collaborator

@trhille trhille left a 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

@xylar xylar force-pushed the update-to-1.7.0-alpha.2 branch 4 times, most recently from 2ff5ff2 to fa44af4 Compare April 8, 2025 04:15
@xylar xylar mentioned this pull request Apr 9, 2025
1 task
xylar added 9 commits April 9, 2025 09:21
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.
@xylar xylar force-pushed the update-to-1.7.0-alpha.2 branch from fa44af4 to 044ace2 Compare April 9, 2025 14:31
@xylar xylar marked this pull request as ready for review April 9, 2025 15:05
@xylar
Copy link
Collaborator Author

xylar commented Apr 9, 2025

@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...

@xylar
Copy link
Collaborator Author

xylar commented Apr 9, 2025

I'll certainly re-test the pr and full_integration before I merge.

@xylar
Copy link
Collaborator Author

xylar commented Apr 9, 2025

Further testing

full_integration and pr suites pass on Chrysalis with gnu and intel, respectively.

Copy link
Member

@matthewhoffman matthewhoffman left a 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

@xylar
Copy link
Collaborator Author

xylar commented Apr 9, 2025

Thanks @trhille and @matthewhoffman!!

@xylar xylar merged commit b0bbf47 into MPAS-Dev:main Apr 9, 2025
7 checks passed
@xylar xylar deleted the update-to-1.7.0-alpha.2 branch April 9, 2025 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up dependencies and deployment Changes relate to creating conda and Spack environments, and creating a load script documentation Improvements or additions to documentation land ice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants