-
Notifications
You must be signed in to change notification settings - Fork 53
Split OHC, temperature and salinity anomalies into separate tasks #294
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
Conversation
TestingSo far, I have tested this on my laptop using the QU240 test case. All plots except the OHC Hovmoller (see note above) look the same as before. I have made some very small formatting changes along the way. |
|
@milenaveneziani, I would appreciate it if you could run a test or two on this new code to make sure nothing is broken. I would also appreciate you comparing the resulting 4 anomaly plots with the same from a previous run of the analysis to make sure you don't have any concerns about the resulting changes. I'm happy to address any concerns you might have. |
NoteI removed the ability to plot "original fields" (as opposed to anomalies) for now. These would be relatively easy to add back in if we thought they would be useful but since they ware off by default and we don't seem to be including them in our workflow, I decided to simplify my work and the default config file but leaving them out. I will happily add them back in if someone thinks they are important. |
| # plot S, T, and OHC fields themselves, in addition to their anomalies? | ||
| plotOriginalFields = False | ||
| # compare to observations? | ||
| compareWithObservations = False |
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 see this has been removed: we were probably no longer using compareWithObservations anyway. At some point, though, we will want to process the OHC observations for transient/historical runs. This will only make sense for those runs, so we will need some sort of flag to turn on/off the comparison with obs.
|
@xylar, I was testing this on edison, using edison_acme_unified_2017.9.26, and got the following error: |
|
Both temp and salt anomaly trends went fine though. I guess that's because temperature and salinity are not xarray datasets? |
|
Thanks, @milenavezeiani. I'll look into the error. I presumably need to disable dask somehow (it should be necessary for these fields anyway). |
|
why did it work on your laptop though? different dask/xarray version? |
|
I think it probably has to do with the size of the dataset but I also bet there's a trick for forcing xarray not to use dask. Otherwise rolling is a pretty useless feature... |
|
@milenaveneziani, the answer seems to be that I'm using xarray 0.10.0 on my laptop, whereas we're still using 0.9.6 on supported machines. I'm going to see about deploying that newer version of xarray in e3sm-unified. I'll keep you posted as soon as I have something on Edison. The issue was addressed in this xarray PR: |
ffdf131 to
63e7afc
Compare
|
@gstreletz, I finally got around to splitting the work you did into several tasks and subtasks. I would appreciate it if you could take a look at what I've done and let me know if you have any concerns. For the moment, I don't have tasks to plot the "original" fields (non-anomalies) as you will see above, but it wouldn't be hard at all to add back in support. I just haven't got to it yet (in part because we don't make those plots by default). |
|
Re: original fields: I think it is OK to leave them for the future, because on the global scale and for the coupled system, we usually look at the trends of the anomalies. What do you think about my comment on the |
Yes, I think that's right. If you try
I don't think it's good to have flags in the config file for features that aren't yet supported. There is no reference to |
ok, I will try my old test with the new conda and see how things go.
ok, that was my only concern. Agreed that we shouldn't have unused flags in the config file. |
|
The new test passed. I do not see any differences between the results that come out of this branch and the ones from develop, at least visually. Edit: the only difference I see seems to be due to a different aspect ratio of the new plots. You can check for yourself: |
|
I turned of the MOC and my test completed as expected: |
|
Waiting to merge this until I can rebase onto develop after #296 is merged. |
|
@milenaveneziani, thanks very much for the testing. |
66a8dc0 to
38e1c1b
Compare
| module unload python | ||
| module use /ccs/proj/cli115/pwolfram/modulefiles/all | ||
| module load python/anaconda-2.7-climate | ||
| module use /ccs/proj/cli127/software/modulefiles/all |
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.
@xylar: what is cli127? I don't have access to it.
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.
Hmm, it's the only group I have access to. Maybe our alcc project?
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 don't have access to cli115. Is that the E3SM group on olcf?
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.
yep, it's the main e3sm project at OLCF. You should have access to it. Could you ask for it? it should be fairly quick: usual form and Mark Taylor is the PI.
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.
Okay, so I'll move this to cli115 as soon as I have permission.
|
as expected, tests passed on edison when the new e3sm-unified conda is used. |
|
I now have rhea access so I'll set up e3sm-unified soon. |
|
/ccs/proj/cli115 appears to be full so I'm not able to install e3sm-unified there. I believe an acme_diags test is at least partly to blame. I have posted a confluence note for @chengzhuzhang. |
really.. I also have a lot of stuff in there that I can't really delete (v0 processing).. I didn't get any notice that the space was full or had any problems recently. Edit: that's the shared space that is full. OK, I have nothing in there. |
554e881 to
8f9f644
Compare
08a7d62 to
9304c77
Compare
If an MPAS dataset is saved back to disk, it will have a Time variable rather than xtime (or its variants). Support has been added for reading back such files.
These new functions compute the moving average of a data set over a window of samples in time, and the anomaly of that moving average from the beginning of an MPAS simulation.
The user would now supply a list of regions to plot and wouldn't need to know which indices they correspond to. This is more intuitive for a user to change in the future.
Sometimes, a moving average is not needed and other times the moving average has already been performed on the data before plotting.
The temperature and salinity tasks are now separate from the OHC task. Also, three new subtasks have been added to perform the 3 anomaly analyses, one for computing a moving average of an anomaly and two for plotting Hovmoller plots and depth-integrated time series of the resulting anomaly.
This is needed because we now require xarray>=0.10.0
9304c77 to
9c42d4b
Compare
|
@milenaveneziani, I now have e3sm-unified/1.1.2 everywhere, which includes both the version of xarray needed by this PR and the version of nco needed by #300. Could you retest on titan or rhea using the instructions on the Supported Machines confluence page? |
milenaveneziani
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.
I have tested on titan after loading the new e3sm-unified conda and all worked fine.
|
@milenaveneziani, great! Thanks so much for testing so late. |
Three new subtasks have been added to perform the 3 anomaly analyses, one for computing a moving average of an anomaly and two for plotting Hovmoller plots and depth-integrated time series
of the resulting anomaly. The anomaly data is now stored in the
timeSeriesoutput folder so it can be shared across plotting subtasks and for provenance.New utility functions have been added to compute moving averages and anomalies.
In several tasks,
regionIndicesToPlothas been replaced byregions, a list of the region names instead of the region indices. This should be more intuitive for a user to change.Note: Plots should be qualitatively the same as they were before. However, I have seen visible differences in the OHC Hovmoller plot. I believe this is due to a slightly incorrect computation of the anomaly previously and that the current plot is correct. The previous computation assumed that only the layer temperature changed with time, whereas the current computation accounts for the fact that other fields (namely layer thickness) that contribute to the OHC can potentially change with time.