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

Improve model vs. obs transects on the native MPAS mesh #851

Merged
merged 6 commits into from
Jan 23, 2022

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Jan 22, 2022

After some discussion below, the most straightforward solution seems to be to remove the line between the topography and the colormap, and make the NaN color the same as the background color.

image

closes #849

@xylar
Copy link
Collaborator Author

xylar commented Jan 22, 2022

Testing

I ran SOSE, WOCE and MOC analysis, and they all look okay with these changes (the MOC is unaffected, as we want it to be). These are the only standard types of analysis that uses transects.

https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xylar/test_fix_mpas_transect_obs/20210614.A_WCYCL1850-DIB-ISMF_CMIP6.ne30_oECv3wLI.DIBbugfix.anvil/transects_no_line_yrs1-10/ocean/

I also ran the full test suite:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xylar/analysis_testing/chrysalis/fix_mpas_transect_obs/

Things look as expected.

@xylar xylar requested a review from milenaveneziani January 22, 2022 02:05
@xylar xylar self-assigned this Jan 22, 2022
@xylar xylar added the clean up label Jan 22, 2022
@xylar
Copy link
Collaborator Author

xylar commented Jan 22, 2022

@milenaveneziani, could you take a look at this at your earliest convenience and let me know what you think? I'd like to have this fix in the next release sometime next week.

If you prefer, we can switch back to doing the comparison on the obs grid instead of the MPAS mesh.

@milenaveneziani
Copy link
Collaborator

Hi Xylar (I know, it's late, even for our standards..). I do like the MPAS panel but I have to admit that I find all the colors used for the topography and ice shelves a bit distracting. Would it be easy to do the following: 1) leave the upper panel as is (but perhaps use just one color, the light blue, to indicate the ice shelf? I figure you have a reason for using white and blue though); 2) choose one color (darker brown) to mask all the nans in the middle and lower panels, without even using the black contour to delineate the topography. We would basically just show the obs values and the bias only where the obs are available, while still preserving the full model section.

I think having the sections on the native mesh is great, so we should definitely push to keep this!

@xylar
Copy link
Collaborator Author

xylar commented Jan 22, 2022

Thanks @milenaveneziani, I'll keep working on it.

The white vs. the light blue is really the distinction between where the model thinks there is an ice shelf and where we have just depressed the sea surface so there's a smooth transition in sea surface height. I wanted to highlight this in part because I think it might be problematic, but I don't have a problem making that whole region blue instead.

The difference between the dark and light brown is that the light brown is where there are NaNs but the dark brown is where there is simply no mesh. It turns out to be surprisingly difficult to outline the combined light and dark brown regions. If I do a contour, I only get the boundary where the light brown touches the valid data.

Anyway, I agree with your suggestions and I will keep working on how to make them happen.

@pep8speaks
Copy link

pep8speaks commented Jan 22, 2022

Hello @xylar! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-01-22 21:56:00 UTC

@milenaveneziani
Copy link
Collaborator

It turns out to be surprisingly difficult to outline the combined light and dark brown regions.

What if you don't outline it all, just use one color for both. Oh maybe that's what you did in the last commit..

@xylar
Copy link
Collaborator Author

xylar commented Jan 22, 2022

Without an outline, it's hard to tell that the bathymetry isn't just part of the dataset. I think the outline really does help so I'll work on it...

@xylar
Copy link
Collaborator Author

xylar commented Jan 22, 2022

Maybe I'll try the version without an outline and see if that's okay. It's by far the easiest solution.

@xylar xylar force-pushed the fix_mpas_transect_obs branch from 5a83d81 to 1f48404 Compare January 22, 2022 18:08
@xylar
Copy link
Collaborator Author

xylar commented Jan 22, 2022

@milenaveneziani, I think it maybe works okay:

image

Do you think I should keep the line between the ice shelf and the rest? I could go either way. The light blue is similar to colors in many of our color maps so the line might keep thing clearer. But it also is kind of odd to have it there and not around the topography.

@xylar
Copy link
Collaborator Author

xylar commented Jan 22, 2022

@xylar
Copy link
Collaborator Author

xylar commented Jan 22, 2022

Here's a version without the line below the ice shelf:
image

And the rest of the plots in that style:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xylar/test_fix_mpas_transect_obs/20210614.A_WCYCL1850-DIB-ISMF_CMIP6.ne30_oECv3wLI.DIBbugfix.anvil/transects_no_line_yrs1-10/ocean/

I think maybe I do like that better and I'll commit that change.

@xylar xylar force-pushed the fix_mpas_transect_obs branch from 1f48404 to acbc9fa Compare January 22, 2022 18:27
xylar added 6 commits January 22, 2022 15:55
This was previously documented as being a RemapMpasClimatologySubtask
object, but this is the parent class and is missing some required
attributes (notably `remap`).
This allows both the MPAS topography and the obs or SOSE topography
to be displayed.
We plot ice where the landIcePressure is depressing the sea
surface, not just where the landIceFraciton is nonzero.
Also working on how to get the expected outline
@xylar xylar force-pushed the fix_mpas_transect_obs branch from acbc9fa to 9de4f2b Compare January 22, 2022 21:55
@xylar
Copy link
Collaborator Author

xylar commented Jan 22, 2022

@milenaveneziani, this is ready for you to give it another look as soon as you have time.

@milenaveneziani
Copy link
Collaborator

Yes, I agree that your last version works best. Nice work Xylar!

@xylar
Copy link
Collaborator Author

xylar commented Jan 23, 2022

Thanks @milenaveneziani.

@xylar xylar merged commit 9a19698 into MPAS-Dev:develop Jan 23, 2022
@xylar xylar deleted the fix_mpas_transect_obs branch January 23, 2022 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Obs look weird on transect on the native MPAS grid
3 participants