-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add OSNAP Arctic Transects transport time series #910
Conversation
@anirban89, I think this still has the default title from the name of the branch. Could you change it to be a little more informative? |
This will need MPAS-Dev/geometric_features#185 to be merged, a new release of |
@xylar yes I agree this PR does not need to be merged right now |
@alicebarthel and @milenaveneziani, we want to add obs. to this. What is the right upper and lower value we should use? Min and max in time? |
@xylar: sorry, I never replied to this. I usually compute a standard deviation from all the obs I have (or get it from previous literature) and then plot that as +-sigma around the mean value. @alicebarthel: what do you think? |
@xylar I have a commit that adds the observational range to this PR (and I've rebased onto develop). After that, this PR should be ready to merge. How should I proceed? I am not able to push to @anirban89's fork. |
3f8e0a9
to
59fb7b1
Compare
@xylar @milenaveneziani @alicebarthel This is ready for review. I don't think all of you need to review it, just maybe whichever one of you has time and interest. |
I'm running my suite of tests on this now. Thanks for reviving it @cbegeman! |
It seems like the docs may need a bit of an update: |
Thanks @cbegeman! I probably won't get to it today but hopefully tomorrow. |
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.
Other than the docs, I think this is ready to go. Here are some test results at QU240 res:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xasay-davis/analysis_testing/chrysalis/add_osnap_voltrans_ts/main_py3.11/ocean/index.html#transporttime
Here are the full test results, which all seem okay:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xasay-davis/analysis_testing/chrysalis/add_osnap_voltrans_ts/
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.
Thanks @cbegeman, these look great! (the tasks and code, not the sad transports through osnap)
Approving based on visual inspection and a short 10-yr analysis test, provided the minor doc updates that Xylar already pointed at.
@xylar This is ready for you to merge when you get back from vacation. |
Thanks @anirban89, @cbegeman, @alicebarthel and @milenaveneziani! |
Add Arctic Transects from branch https://github.com/milenaveneziani/geometric_features/tree/flipOSNAP_renameArcticTransectsTag of geometric features.
Separate all arctic transects from standard transects using transectGroup flag in config option
timeSeriesTransport
.When running
mpas_analysis
without the flag--polar_regions
only standard transects are plotted. All Arctic transects including the three new ones "Hudson Bay-Labrador Sea", "OSNAP section East" and "OSNAP section West"are only plotted when
--polar_regions
is specified.timeSeriesTransport split into transportGroups "Transport Transects" and "Arctic Transport Transects". By default all Arctic transects are removed from standard transects.
User can specify the list of standard transects to plot with transectsToPlot under config option (default.cfg)
timeSeriesTransportTransects
and leave it asall
undertimeSeriesArcticTransportTransects
. List can not include an Arctic Transect.Similarly user can also specify the list of arctic transects to plot with transectsToPlot under config option
timeSeriesArcticTransportTransects
but to plot thesempas_analysis
needs to be run with the--polar_regions
flag.Files changed:
default.cfg
,polar_regions.cfg
andocean/time_series_transport.py
@xylar @milenaveneziani @alicebarthel