-
Notifications
You must be signed in to change notification settings - Fork 53
Use ncrcat to extract time series #271
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
|
@milenaveneziani, this should be the last PR for v0.6. (Unless, of course, there are bug fixes.) I will do some testing today and post the results along with a suggestion for a test you can run as a sanity check. |
|
Should address #225 |
f3d4b94 to
cc31860
Compare
|
I discovered a bug in testing this branch, #273. While the bug isn't directly related to this PR, I think it should be fixed before we continue with testing, since it's hard to tell if this branch is behaving properly until we fix the other bug. |
cc31860 to
13808d9
Compare
TestingUbuntuRuns successfully EdisonBatch mode: memory error during MOC AnvilBatch mode and login node: memory error during MOC GrizzlyBatch mode: memory error during MOC ThetaBatch mode: run did not finish in allotted hour |
|
MOC Error: |
|
I'm going to see if switching from |
|
If that doesn't work either, we may have to load the previously computed annual averages, instead of the monthly means from the timeSeriesStats files. That would do, for a post-processed MOC timeseries, I think. And hopefully it will be less likely that we get into memory errors.. |
|
Okay, so it's technically working but it seems like a giant waste of time to first copy GB upon GB of data (164 GB for the example on Edison I'm working with, and that's only 22 years of MOC at EC60to30 res.) before the processing can even begin. It would probably make more sense to read directly from each monthly file in the particular case of the MOC time series. I'll look into it... |
|
For what it's worth, it worked... |
Testing UpdateUbuntuRuns successfully EdisonBatch mode (all) and login node (MOC only): runs successfully AnvilBatch mode: runs successfully GrizzlyBatch mode: runs successfully ThetaBatch mode: errors in several tasks. However, Theta seems to be having issues or be down for maintenance or something so I am not convinced the errors are related to this PR. |
|
@milenaveneziani, if you want to run a test of your own, please do so. I think this is finally ready to merge. |
|
Oops, some more debugging to do. If I rerun without the |
060e21a to
64a37b7
Compare
|
I think I have the error fixed but some more testing is needed. Probably will have to wait until tomorrow. |
|
I will do some testing of this tonight. Thanks @xylar. |
|
Okay, that would be good. I have a feeling we are likely to expose some more bugs by doing further testing. Better sooner than later. |
|
The theta run worked in the end, though it required 1 hour as a batch script (the maximum allowed on a single node) followed by a run on the login node to finish up. All my tests have now passed, including re-running an analysis job without the --purge flag. |
|
I have just started a test on titan. Quick question: how many years did you ask for when computing time series, in your various tests? |
|
I tested time series of the following lengths:
|
|
@xylar: unfortunately the two subsequent tests I did today did not work as desired. Here is a summary. I used the same beta2 run as before, but saving stuff in a different directory, and asked for computing timeseries tasks only (in my previous tests I was computing everything). My first test had year_start, year_end = 1, 10 and all went well as expected. Then, I did two more tests without purging:
and I already knew something was wrong because it did not go through ncrcat. didn't get the same error for SST timeseries, but I believe nothing happened there (timeseries was not updated to go from year 5 to 15). After this, I canceled the test.
I am not sure what causes the OHC error in 1), but I am thinking these issues are all related to the fact that, for some reason, ncrcat is not called again after the very first run. I wonder what happens if I choose a period that does not overlap at all with the first one. I will try this case now. |
|
ok, so, this last test was for year_start, year_end = 11, 20, i.e. no overlap with previous run. |
This is accomplished through a new task, MpasTimeSeriesTask, which allows other tasks to add variables and then extracts requested variables with a single call to ncrcat (per component). All time-series tasks except MOC have been updated to used this functionatly. It may also make sense to use this functionality for MOC but, since it requires substantially more data than other tasks, this has been left out for the time being.
This is because we don't want to slow down other tasks with the potentially very large time series that needs to be extracted for MOC.
This is an alternative that opens a single data file but still parses the MPAS time variable as before and can subset the Time dimension using a start and end date.
Some observational and pre-processed datasets still use the old generalized_reader to open multiple files and process the xtime variable.
Instead, opening each monthly file one at a time to compute each data point in the MOC time series.
Fix a bug in how the final date is extracted
b65a6b8 to
e01d99c
Compare
|
Thanks @milenaveneziani, I will try all of these things myself and see if I can make some progress. |
|
Enjoy your holiday! |
The first year is now always included in the time series, allowing for anomalies to be computed. A bug in computing which dates were already present in the time-series file has been fixed (adding lists behaves very differently from adding numpy arrays!).
This merge fixes a bug in renaming variables in the first-year data set if one is created. Previously, not all variables in the renameDict were included in the data set (since not all are used) but now all are kept for simplicity.
|
@milenaveneziani, I don't expect you to work on this at all over the long weekend. I just wanted to let you know that I think I have fixed the 3 issues your testing showed. Here is a summary:
I'm sorry for not doing a better job of testing my own code and for making you discover errors that I should have uncovered myself. Thanks again for your help. |
|
By the way, because of the way |
|
@xylar: I re-did the 3 tests listed above and this time all went well. Great! Only one question left for me: the MOC time series do not remember previous runs at all, even if I ask for the exact same number of years. Is that to avoid memory problems with ncrcat? One other thing: I haven't tested in batch mode at all. Let me know if you want me to do one such test on some machine. |
|
@milenaveneziani, thanks for re-testing! Ah, I'm glad you noticed that problem with the MOC. That should be easy to fix. When I switched to using ncrcat for the MOC data I must have messed up the check for the stored time-series data. I think it's perfectly fine to check in batch mode. Presumably we want to do both login-node and batch-mode testing from time to time but I don't think it's necessary for every PR. |
Cache the MOC time series to an output file and only compute time entries that are not already cached. Also, some very minor PEP8 clean-up in an unrelated file.
|
@milenaveneziani, could you run a test where you look at MOC time-series caching again? You can just run with the
Hopefully, you see the same kind of behavior |
|
@xylar: thanks for the latest changes. I'd say you can merge this. |
|
@milenaveneziani, thank you so much for doing this testing over the holiday! |
Clean up the nino34 index task This clean up is needed to generalize the task in preparation for supporting comparison with a reference run (which will replace one of the panels). A new task has been added to extract time series needed by climate indices (which may have different time bounds than time series). Support for separate time bounds was eliminated, perhaps accidentally, in #271. Nino34 spectra are now packed into dictionaries for easier iteration and panels are plotted in loops. The function for determining the maximum value of all spectra (to determine the bounds of the y axes of these plots) has been cleaned up so it no longer relies on a plot already having been performed). All warnings previously produced with warnings.warn have been switched to just using print('Warning:..') because this produces much more intuitive output without a (nearly always useless) stack trace.
This is accomplished through a new task,
MpasTimeSeriesTask, which allows other tasks to add variables and then extracts requested variables with a single call toncrcat(per component). All time-series tasks except MOC have been updated to used this functionality.For the MOC time series, monthly mean files are being opened one by one as separate data sets (saving the trouble of creating a combined data set only to break it back up into time slices).
All MPAS data sets are now opened with
open_mpas_dataset, which parses time fromxtimeorxtime_startMonthlyandxtime_endMonthlybut doesn't try to combine multiple files. This seems to spare us from may of the problems we've run into with dask in the past.