Switch to mosaic for all plotting functionality
#256
Conversation
|
@andrewdnolan, could you change "Fixes #255" to "Partially addresses #255" since we don't want to add python 3.13 testing in this PR. If you leave things as they are, merging this PR will automatically close #255. |
TestingSetup for testing: diff --git a/polaris/ocean/tasks/barotropic_gyre/forward.py b/polaris/ocean/tasks/barotropic_gyre/forward.py
index 2424bdc5d..4ab854740 100644
--- a/polaris/ocean/tasks/barotropic_gyre/forward.py
+++ b/polaris/ocean/tasks/barotropic_gyre/forward.py
@@ -139,7 +139,7 @@ class Forward(OceanModelStep):
output_interval_str = time.strftime('0000_%H:%M:%S',
time.gmtime(run_duration))
else:
- stop_time_str = time.strftime('0004-01-01_00:00:00')
+ stop_time_str = time.strftime('0001-04-01_00:00:00')
output_interval_str = time.strftime('0000-01-00_00:00:00')
replacements = dict(You'll also need a config file (below named [barotropic_gyre_default]
steps_to_run = init short_forward long_forward analysisSetting up the baseline: # with the head of E3SM's main branch checked out
source <POLARIS_DIR>/load_dev_polaris_0.5.0-alpha.1_pm-cpu_gnu_mpich.sh
# 9: ocean/planar/barotropic_gyre/default
# 11: ocean/planar/ice_shelf_2d/5km/z-star/default/with_viz
# 65: ocean/spherical/icos/cosine_bell/convergence_space/with_viz
polaris setup -n 9 11 65 -w $PSCRATCH/polaris_PR#256/main/ -f custom.cfg
pushd $PSCRATCH/polaris_PR#256/main/
salloc --nodes 3 --qos interactive --time 01:00:00 --constraint cpu --account m4259
bash job_script.custom.sh
# kill job once it's done Testing this PR's branch: cd <POLARIS_DIR>
git checkout mosaic_viz
source <POLARIS_DIR>/load_dev_polaris_0.5.0-alpha.2_pm-cpu_gnu_mpich.sh
polaris setup -n 9 11 65 -w $PSCRATCH/polaris_PR#256/devel/ -b $PSCRATCH/polaris_PR#256/main/ -f custom.cfg
pushd $PSCRATCH/polaris_PR#256/devel/
salloc --nodes 3 --qos interactive --time 01:00:00 --constraint cpu --account m4259
bash job_script.custom.sh
# kill job once it's done Then, if you want to pull the all of the figures generated by the test suite locally I ran: ssh perlmutter.nersc.gov 'find /pscratch/sd/a/anolan/polaris_PR#256 -name "*.png"' | rsync -vP --files-from=- perlmutter.nersc.gov:/ ~/Desktop/ |
Testing: Spherical ResultsFile to compare: main:
develop: |
Testing: Planar ResultsFile to compare: main: develop: |
Testing: Planer Results (at vertices)File to compare: Note: the error is really large because I only ran this for 3 months, instead of 3 years. main: develop: |
xylar
added
framework
There was a problem hiding this comment.
@andrewdnolan, this looks great!
I do have some suggested changes based just on a code review so far.
Please add mosaic to the intersphinx part of the docs/conf.py file:
https://github.com/andrewdnolan/polaris/blob/mosaic_viz/docs/conf.py#L74
This will allow links to mosaic classes from the documentation (notably the autogenerated API docs).
@cbegeman, can you comment on whether you're comfortable with switching the barotropic gyre plots from contourf to continuous for now? mosaic doesn't (yet?) support contourf.
|
One more thing, we will need to drop python 3.9 support since |
|
@andrewdnolan, a couple of my suggested renaming changes need other changes to go with them that I didn't try to make. Please add those as well, as things are broken right now. |
@xylar I've got those changes locally, going to remove the |
|
@andrewdnolan, a quick rebase seems to be needed to fix conflicting updates in After that, I'm happy to run a few tests and then I think I'll be able to approve. |
|
@xylar I still need to update the > git diff --name-only origin/main | grep polaris/ocean/tasks | xargs dirname | sort -u
polaris/ocean/tasks/baroclinic_channel
polaris/ocean/tasks/baroclinic_channel/rpe
polaris/ocean/tasks/barotropic_gyre
polaris/ocean/tasks/cosine_bell
polaris/ocean/tasks/geostrophic
polaris/ocean/tasks/ice_shelf_2d
polaris/ocean/tasks/inertial_gravity_wave
polaris/ocean/tasks/manufactured_solution
polaris/ocean/tasks/sphere_transportAs a final step I'll run a suite with all of those. If you're OK with it, I'm not going to do a baseline like above. I'm just going to run with my branch and do visual inspection that everything looks OK. |
| masked_cells = ds_mesh.nCells.where(~cell_mask, drop=True).astype(int) | ||
|
|
||
| # use inverse so True/False convention matches input cell_mask | ||
| edge_mask = ~cells_on_edge.isin(masked_cells).any("TWO") |
There was a problem hiding this comment.
Does this mean that there has to be unmasked cells on either side of an edge for it to be valid?
There was a problem hiding this comment.
Yes, there does have to be unmasked cells on either side of an edge, but not both, for it to be valid.
With the cells_on_edge array being zero based, boundary edges will have one entry that's >= 0 and one -1 entry. The int array masked_cells is zero based as well, so a value of -1 can never be in it. So, the isin condition will never be true for the missing cell along boundary edges. Therefore, the edge_mask will only ever be true for a boundary edge if the one valid cell on the edge is True in the cell_mask that's passed to the function.
@andrewdnolan Can we switch back to the discrete colorbar here? I chose that intentionally so that the streamlines were more clear. |
|
@andrewdnolan, to get what @cbegeman is asking for, I think it should be possible to just coarsely sample the colormap to get the same effect as |
Discrete colormap is sufficient. Thanks! |
|
There was some offline discussion, but as of 0759678, the barotropic stream function plotting routine uses a discrete colorbar with 21 level for the first two pannel, producing: |
|
@xylar I've updated the |
Testing: full suite of tasks altered by this PRSetup for testing: diff --git a/polaris/ocean/tasks/barotropic_gyre/forward.py b/polaris/ocean/tasks/barotropic_gyre/forward.py
index 2424bdc5d..4ab854740 100644
--- a/polaris/ocean/tasks/barotropic_gyre/forward.py
+++ b/polaris/ocean/tasks/barotropic_gyre/forward.py
@@ -139,7 +139,7 @@ class Forward(OceanModelStep):
output_interval_str = time.strftime('0000_%H:%M:%S',
time.gmtime(run_duration))
else:
- stop_time_str = time.strftime('0004-01-01_00:00:00')
+ stop_time_str = time.strftime('0001-04-01_00:00:00')
output_interval_str = time.strftime('0000-01-00_00:00:00')
replacements = dict(You'll also need a config file (below named custom.cfg) which contains: [barotropic_gyre_default]
steps_to_run = init short_forward long_forward analysis
[inertial_gravity_wave_convergence_space]
steps_to_run = init_200km forward_200km_300s init_100km forward_100km_300s init_50km forward_50km_300s init_25km forward_25km_300s analysis viz
[manufactured_solution_convergence_space]
steps_to_run = init_200km forward_200km_150s init_100km forward_100km_150s init_50km forward_50km_150s init_25km forward_25km_150s analysis vizTesting this PR's branch: # 0: ocean/planar/baroclinic_channel/10km/default
# 4: ocean/planar/baroclinic_channel/10km/rpe
# 9: ocean/planar/barotropic_gyre/default
# 11: ocean/planar/ice_shelf_2d/5km/z-star/default/with_viz
# 28: ocean/planar/inertial_gravity_wave/convergence_space
# 59: ocean/planar/manufactured_solution/convergence_space
# 65: ocean/spherical/icos/cosine_bell/convergence_space/with_viz
# 77: ocean/spherical/icos/geostrophic/convergence_space/with_viz
# 113: ocean/spherical/icos/rotation_2d/with_viz
polaris setup -n 0 4 9 11 28 59 65 77 113 \
-w $PSCRATCH/polaris_PR#256/devel_full_suite/ \
-f custom.cfg
pushd $PSCRATCH/polaris_PR#256/devel_full_suite/
salloc --nodes 3 --qos interactive --time 01:30:00 --constraint cpu --account m4259
bash job_script.custom.sh |
Results : full suite of tasks altered by this PRA bunch of tasks fail, but I don't think for visualization related reasons for any of them.... Results:
Seems to be an issue with running this as an interactive job, maybe? The task is not using the full path to read in the
Again another issue finding the files, not with plotting. Looks like the
The convergence rate is way out of wack here, do I need to be using a specific branch of mpas-ocean to get this to pass? Not totally sure why the |
|
@andrewdnolan, I'll look into the first two failures. Those are bugs, I'm pretty sure.
I think that's expected. We usually test
Yes, that's by design. The |
|
I think it should be min(xEdge) to min(xEdge)+xPeriod. |
|
That makes sense to me. I'll open PR in |
- Interface to the `plot_horiz_field` has been simplified slightly. - The mask array now must be the same shape as the input field array. - Added a framework function to convert a cell mask to and edge mask.
Also simplified the config options for the colorbar options, which makes the spherical and lat/lon plots have the same options
Dependencies for `xarray` based visualization have been removed
Co-authored-by: Xylar Asay-Davis <[email protected]>
- Renamed all instances of cell_mask_to_edge_mask and added to API - Fixed typo with adding cartopy features to spherical plots - Added mosaic to intersphinx mapping
andrewdnolan
force-pushed
the
mosaic_viz
branch
from
0759678 to
f4ffbb7
Compare
|
@xylar I just rebased this branch onto main, which required bumping the alpha version from 2 to 3. I've also constrained the |
|
@andrewdnolan I think something might be going wrong with kite selection at edges. The left and right sides don't fit together like puzzle pieces to my eye (if you try to wrap the domain hot dog-wise) |
|
@cbegeman, I think this is working correctly in that the cropping bounds are exactly what we want them to be. There are some edge kites that are entirely outside of this range, but that is the fault of |
|
@xylar and @cbegeman Yes, that is correct. A limiting assumption that we make in I've done some initial prototyping of mirroring the patches across periodic boundaries, and these "incorrectly" placed edge patches would implicitly be corrected in that mirroring. I haven't had a chance to really finish that work, since it requires a decent amount of code additions (the array length is changing with mirrored patches, which has to be handled under the hood). Because this (i.e "incorrectly" placed edge patches) will be fixed with mirror, Xylar and I opted for one all encompassing solution than a special condition for these edge patches. But if it's seems like a necessity for this PR, I can look into a short term solution to tide us over. |
|
For what it's worth, this is the version of the figure that's produced using the current
So the dangling kites on the right hand side also appear to be a problem in the current version of the polaris viz utilities. With this PR, we are now at least setting the periodic axis limits correctly. There's still some work do to on the |
|
@andrewdnolan and @xylar Thanks for catching me up on what you've been thinking. The main thing I'm concerned with is all the edges being plotted in time for us to catch any periodic boundary issues that might emerge with Omega. Can you clarify what you mean by
Do you mean that there is a way to plot the figure you showed above with the current main even after this PR goes in? If not, and we are losing the ability to plot some edge kites, what would the timeline be for patch mirroring? |
|
Oh, sorry I guess I misinterpreted what what was requested above (circa 2 weeks ago). @cbegeman Until we can mirror patches across the periodic boundary, is it preferable to keep the whole kite visible (even if they extend beyond the periodic axis limit)? This contrasts with "tight" axis implemented in 394fa42, which set the periodic limits based on the cell patch extent and cuts off any patches that cross the periodic boundary. With the "tight" axis limits these "incorrectly" places edge patches appear to be missing, because they lie entirely over the periodic axis limit. To clarify what I mean above; I was just trying to highlight that the current polaris viz functions also have this problem of "incorrectly" placed edge patches. With 394fa42, we were "tightly" setting the axis limits based on the periodic extent and in turn entirely cutting off the "incorrectly" placed patches. We can undo the "tight" axis setting, so that the axis limit match the what's producing using the |
|
@andrewdnolan I'm sorry for the confusion and for not following this PR more closely. My preference is for "incorrectly" placed edges with no information loss over the tight axis. So if you're ok with it, I'd prefer to have the tight axis off by default. I do think your patch mirroring solution will be great. I didn't realize that main had incorrectly placed edges; not sure what changed from the first merge of this capability (e.g., #33 (comment)) but that's water under the bridge. |
|
@cbegeman No worries! That's an easy enough fix on my end. I think in the figure linked, these "incorrectly" placed edges are masked out. In the current version, if focus on the bottom right corner with the patch with it's leftmost node at exactly 150 km, you'll notice two edge patches to the right of that edge (and one of those is an "incorrectly" placed edge). Both of those seem to be missing in the figure you linked above. Also the left x-axis limit is greater in the linked version of the plot than the version here. Looks like the current version of the edge plotting was done in #150. Anyway, I think once I loosen the axis limit this PR should be just about complete. I'm planning to talk through some of the lingering details with regards to mirror patches and we should have that implemented soon (hopefully within the next two weeks?). |
There was a problem hiding this comment.
@andrewdnolan Great work! Approving after agreeing on a way forward with those edge kites. I'm excited about this switch!
|
Tight axis limits have been undone as of 9684bea. I just commented out the code to handle the tight axis limits (instead of deleting), since we'll hopefully have the mirroring in Here's what the barotropic gyre figure now look like, using this branch: |
There was a problem hiding this comment.
@andrewdnolan, thanks so much for working so hard on implementing and testing this. I'm happy to approve based on your latest testing.
|
Again, merging despite the expected failures in python 3.9. |
Partially addresses #255 and fixes the plotting issues from #242.
Checklist
api.md) has any new or modified class, method and/or functions listedTestingcomment in the PR documents testing used to verify the changes