Skip to content

Update Earth radius in build_mesh to the CIME value#341

Merged
xylar merged 13 commits intoMPAS-Dev:masterfrom
xylar:consistent_compass_earth_radius
Sep 4, 2020
Merged

Update Earth radius in build_mesh to the CIME value#341
xylar merged 13 commits intoMPAS-Dev:masterfrom
xylar:consistent_compass_earth_radius

Conversation

@xylar
Copy link
Collaborator

@xylar xylar commented Sep 1, 2020

Partly addresses MPAS-Dev/MPAS-Model#549

xylar added 12 commits August 31, 2020 13:10
Move several tools that are specifically for the ocean to
`mpas_tools.ocean`.

Tools related to defining signed distance functions have been
pulled out of `coastal_tools` and remain in
`mpas_tools.mesh.creation`, since they should be generally useful.

In the function lonlat2xyz, the radius has been added as an
optional parameter, and the Earth radius is now a required
parameter for signed distance functions.
Each function now takes as parameters the results of the function
in define_base_mesh, rather than calling that function.  This is
a much cleaner way to pass in the fields and gives users a lot
more flexibility.

Since bathymetry and floodplain are ocean-specific concepts, the
code for adding them has been moved to mpas_tools.ocean and isn't
appropriate to include in build_spherical_mesh anymore.  So
calling code will need to add these steps after calling
build_shperical_mesh.
Add a warning that build_mesh docs are now completely useless.
This particular meaning of meshDensity is a legacy of the
pre-JIGSAW method for mesh generation and is only used in the
ocean core.
This saves us from having to know the name of the output file
in cases where we don't need to write out and read back the
mesh density.
These add meshDensity and optionally add bathymetry and preserve
floodplain
@xylar
Copy link
Collaborator Author

xylar commented Sep 1, 2020

@proteanplanet, @dengwirda, if you could take a look just at the last commit of this PR and let me know if it looks correct to you, that would be helpful. The rest is from #314 and will disappear once that gets merged and I can rebase.

Copy link
Contributor

@dengwirda dengwirda left a comment

Choose a reason for hiding this comment

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

Thanks @xylar --- I followed the instructions in ../conda_package/docs/ and built the full environment without issue.

I can see that the Earth radius now consistently comes from ../mpas_tools/cime/constants.py. Just to double-check, is the idea to use ../mpas_tools/tests/test_cime_constants.py to ensure this stays in-sync with ../E3SM-project/E3SM/cime/?

@xylar
Copy link
Collaborator Author

xylar commented Sep 1, 2020

Just to double-check, is the idea to use ../mpas_tools/tests/test_cime_constants.py to ensure this stays in-sync with ../E3SM-project/E3SM/cime/?

Yes, If you look at tests, which get run every time there's a PR, a merge to master or a new tag, we check that things match E3SM's version of CIME:
https://github.com/MPAS-Dev/MPAS-Tools/blob/master/conda_package/mpas_tools/tests/test_cime_constants.py#L5

The same would be true if we add other constants from CIME in the future.

@xylar
Copy link
Collaborator Author

xylar commented Sep 1, 2020

Oh, shoot! Except, I now see that test is broken but it's not giving an error. Work to do...

mark-petersen added a commit to mark-petersen/MPAS-Model that referenced this pull request Sep 3, 2020
…nto ocean/develop

Update Earth radius to the CIME value MPAS-Dev#680

Currently, COMPASS uses 3 different Earth radii. With this merge and a
corresponding update to MPAS-Tools, all Earth radii are the CIME value.

This PR addresses MPAS-Dev#549 along with MPAS-Dev/MPAS-Tools#341.
@xylar xylar removed the request for review from proteanplanet September 4, 2020 05:48
@xylar
Copy link
Collaborator Author

xylar commented Sep 4, 2020

Implicitly approved by @mark-petersen and @sbrus89 via MPAS-Dev/MPAS-Model#680

@xylar xylar merged commit 52d4ede into MPAS-Dev:master Sep 4, 2020
@xylar xylar deleted the consistent_compass_earth_radius branch September 4, 2020 05:49
@proteanplanet
Copy link

Sorry @xylar, it wasn't clear whether you'd finished making your changes. So I held off.

@xylar
Copy link
Collaborator Author

xylar commented Sep 4, 2020

@proteanplanet, sorry about that. I will try to make that clearer in the future by keeping my PRs either as drafts or as "in progress" until they are ready to review and pinging you when the time comes.

I am moving on with this one pretty fast because it is holding up E3SM v2 critical work.

mark-petersen added a commit to MPAS-Dev/MPAS-Model that referenced this pull request Sep 9, 2020
Update to version 0.1.11 of the compass environment #688

This brings in several changes from MPAS-Tools:

* Changes to make sure the Earth radius is consistent throughout COMPASS
  and `mpas_tools.ocean` by using the CIME value
  (MPAS-Dev/MPAS-Tools#347 and
  MPAS-Dev/MPAS-Tools#341)

* Improved performance for interpolation of meshDensity and bathymetry
  (MPAS-Dev/MPAS-Tools#344)

* Reorganization of `mpas_tools.mesh.creation`
  (MPAS-Dev/MPAS-Tools#314)
xylar pushed a commit to xylar/old_compass2 that referenced this pull request Oct 12, 2020
Update to version 0.1.11 of the compass environment #688

This brings in several changes from MPAS-Tools:

* Changes to make sure the Earth radius is consistent throughout COMPASS
  and `mpas_tools.ocean` by using the CIME value
  (MPAS-Dev/MPAS-Tools#347 and
  MPAS-Dev/MPAS-Tools#341)

* Improved performance for interpolation of meshDensity and bathymetry
  (MPAS-Dev/MPAS-Tools#344)

* Reorganization of `mpas_tools.mesh.creation`
  (MPAS-Dev/MPAS-Tools#314)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants