-
Notifications
You must be signed in to change notification settings - Fork 53
Time series edits #1112
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
Time series edits #1112
Conversation
xylar
left a comment
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.
A few suggested changes but this looks great over all!
I'll need to test it on the test suite before I approve it for merging.
| landIceFraction = dsMesh.landIceFraction | ||
| landIceFraction = xr.where(landIceFraction > 0, 1, landIceFraction) | ||
| landIceFraction = -1*(landIceFraction-1) |
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.
| landIceFraction = dsMesh.landIceFraction | |
| landIceFraction = xr.where(landIceFraction > 0, 1, landIceFraction) | |
| landIceFraction = -1*(landIceFraction-1) | |
| openOceanMask = xr.where(dsMesh.landIceMask > 0, 0, 1) |
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 think this would be simpler and make more sense (at least to me). (And then openOceanMask gets used below.)
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 think we want to use landIceMask rather than landIceFraction > 0 but that could be a point of discussion.
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.
Sure, if the variable is available that is fine - I had tried first with landIceFloatingMask but that variable wasn't available for some reason, so I went with landIceFraction following the melt rate example
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.
| cellMask = numpy.logical_and(cellMask, landIceMask == 0) |
It seems like either landIceFraction or landIceMask would work so it doesn't matter too much. landIceFloatingFraction should also work but I don't think that would be the right thing if it differs from landIceFraction -- if grounded ice is present, we want to exclude it, too.
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.
landIceMask seems like the right choice to me
|
|
||
| var = dsLocal[variableName].where(vertDepthMask) | ||
| var_mpas = dsLocal[variableName] | ||
| var_mpas_masked = var_mpas*landIceFraction |
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.
| var_mpas_masked = var_mpas*landIceFraction | |
| var_mpas_masked = var_mpas*openOceanMask |
Co-authored-by: Xylar Asay-Davis <[email protected]>
Co-authored-by: Xylar Asay-Davis <[email protected]>
Co-authored-by: Xylar Asay-Davis <[email protected]>
TestingI ran the test suite after making my suggested change above. Things appear to have worked just fine: |
|
@irenavankova, if you want to make my 2 suggested changes, I think this can go in. |
|
@xylar so turns out there is something odd going on at the shelf plots, both in what I had originally and with your corrections. It is especially clear in the salinity and density plots (Darin was the one to notice it actually). |
|
Thanks for pointing those out. I'll take a look but this may point to needing a better solution to the z coordinate. |
|
@irenavankova @xylar Seems like you two have it covered but ping me if you need help with this. |
|
@irenavankova, I see what the issue is and I have a fix. I'll test it and then push it to your branch. The problem is that we are multiplying the field in the numerator by the open ocean mask but we are not including the mask in the denominator, the totalArea. This means we multiply by a smaller area than we divide by and fields get reduced by a constant fraction (the total open ocean area / total ocean area including cavities). We don't really see that in temperature because the bounds include zero but we really see it in salinity and density because the fields are suddenly out of bounds. |
|
The 240km mesh I use for the test suite isn't great for making sure this is fixed but I don't see any signs of trouble:
|
|
I reran @irenavankova's analysis from above with my fix. It looks good now:
|
This ensures that both the area-weighted field (numerator) and the total area (denominator) include the same masking.
xylar
left a comment
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.
This now looks good to me and I think it's ready to merge!




Small changes to time series:
-Moved map inset on time series to lower left corner so that it doesn't hide the time series
-Changed units GT to Gt for melt flux
-Masked out ice-shelf cavities for calculating regional profiles and tasks depending on those, like Hovmoller. The reason for that is coordinate distortion in the cavities which can be an issue for interpreting layer averaged quantities.
Mpas-analysis for melt rate timeseries:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.vankova/analysis/Sv2p1/diff_melt/SSP3-ENS/clim_2015-2100_ts_2015-2100/html/ocean/index.html
for Hovmoller:
This is with changes:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.vankova/analysis/Sv2p1/hov12/SSP3-ENS/clim_2015-2100_ts_2015-2100/html/ocean/index.html#ocnreghovs_antarcticregions&gid=9&pid=4
This is prior to changes:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.dcomeau/E3SMv2_1/v2_1.SORRM.ssp370_ensmean/yrs2081-2100_bt/ocean/index.html#ocnreghovs_antarcticregions&gid=135&pid=4
Checklist
api.rst) has any new or modified class, method and/or functions listedTestingcomment in the PR documents testing used to verify the changes