-
Notifications
You must be signed in to change notification settings - Fork 66
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
Move ocean
and visualization
conda-package scripts
#597
Move ocean
and visualization
conda-package scripts
#597
Conversation
This merge also fixes a few PEP8 issues with coastal alteration. The original scripts are left as stubs that call the entry point functions.
This merge also fixes a few PEP8 issues with moc. The original script is left as stubs that calls the entry point function.
Tested as part of MPAS-Dev/compass#881. |
@sbrus89, can I ask you to represent the Ocean team by reviewing this (and MPAS-Dev/compass#881 if you like)? |
You can get these changes by installing the 1.0.0rc2 package from the |
@xylar, sure I can take a look. Thanks for making these changes. |
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.
These changes look good to me. I'm not an expert on the conda changes, but I looked through the modifications to the scripts/entry points and approve by inspection.
@@ -71,12 +71,7 @@ | |||
'landice/mesh_tools_li/create_landice_grid_from_generic_MPAS_grid.py', | |||
'landice/mesh_tools_li/define_cullMask.py', | |||
'landice/mesh_tools_li/interpolate_to_mpasli_grid.py', |
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.
Are changes needed for these other scripts as well?
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.
Those are in #596, which I haven't asked you to review.
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 see, thanks!
cd ${SRC_DIR}/conda_package/ocean/smooth_topo | ||
cd ${SRC_DIR}/ocean/smooth_topo |
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 don't understand the changes in this file and how they relate to the main objective of the PR, but I'm sure that's just because of my relative conda package ignorance.
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.
We were copying the ocean
and several other directories into the conda_package
directory so the tools found there could be included in the conda package. A major purpose of this PR is to no longer make those copies but rather to require that the conda package takes no python code from outside the conda_package
directory. So that change is on purpose.
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.
We can still build the C++ and Fortran tools that are outside of conda_package
but we need to point to them where they are, not where they used to be copied to.
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.
That makes total sense, thanks!
Thanks @sbrus89! I really appreciate the prompt review! |
This merge aims to move 5 scrips used primarily or exclusively by MPAS-Ocean in Compass into the
mpas_tools
package as entry points.The motivation for this is that we are engaging in Python package modernization on all MPAS-Dev and E3SM-Project repos, and in that process we discovered that modern packaging approaches do not support standalone scripts as the old approach did.
The code from 4 of the 5 scripts has been moved into modules within
mpas_tools
(the 5th script, the paraview VTK extractor, was already a stub). They are inside of functions, as required for entry points into a Python package, in all cases. The original scripts are preserved as stubs that call the entry points.The entry points cannot have the same names as the original scripts: the
.py
extention is not allowed.A few fixes have been made to update the coastal alteration and MOC modules for better PEP8 compliance.
The documentation has been updated to reflect the new names of the command-line tools.