Skip to content

Conversation

@xylar
Copy link
Collaborator

@xylar xylar commented Dec 29, 2017

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 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.

...in preparation for adding a panel of a reference run.

A function for helping to determine the extent of the y axis for
spectra has been moved from plotting into this task and has also
been cleaned up/simplified
@xylar xylar added the clean up label Dec 29, 2017
@xylar xylar self-assigned this Dec 29, 2017
@xylar
Copy link
Collaborator Author

xylar commented Dec 29, 2017

@vanroekel, if you could look over these changes and make sure that you are okay with them, I would appreciate it.

@xylar
Copy link
Collaborator Author

xylar commented Dec 29, 2017

@vanroekel and @milenaveneziani, I am not sure we really discussed the decision to have IndexNino34 use the output from MpasTimeSeriesTask (meaning it has the same bounds as all other time series). This could be reconsidered, or a separate instance of MpasTimeSeriesTask could be used to extract time series for indices (with potentially different bounds). The purpose of removing the [index] section here was to make the config files consistent with the current implementation. An alternative would be to make the code consistent with the config files instead.

@xylar
Copy link
Collaborator Author

xylar commented Dec 29, 2017

Testing

I tested this with the QU240 test case on my laptop. Results are nearly identical with only some small variation in the y axis of the spectra and the format of the titles and legends.

@vanroekel
Copy link
Collaborator

@xylar I think we should have separate bounds for NINO34. As an example, often we have found a need to ignore the first 10 years or so for a NINO computation, but want to retain years 1-10 for other time series.

@xylar xylar force-pushed the clean_up_index_nino34 branch from 114e24f to ffb1b27 Compare December 30, 2017 08:43
@xylar xylar added the bug label Dec 30, 2017
@xylar
Copy link
Collaborator Author

xylar commented Dec 30, 2017

@vanroekel, okay, sounds good. I just switched to having separate bounds for indices again. It didn't take long to implement.


from ..shared.io.utility import build_config_full_path

from ..shared.timekeeping.utility import get_simulation_start_time
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't being used

return (colormap, colorbarLevels)


def plot_size_y_axis(plt, xaxisValues, **data):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to IndexNino34 because it's not likely to be used elsewhere.

@xylar xylar force-pushed the clean_up_index_nino34 branch from 136fe77 to 9a4597a Compare December 30, 2017 09:49
The prefix about a UserWarning with a useless stacktrace in front of
warnings produced by warnings.warn are just confusing.
@xylar xylar force-pushed the clean_up_index_nino34 branch from 9a4597a to 3a76c8d Compare December 30, 2017 11:38
Copy link
Collaborator

@vanroekel vanroekel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xylar changes look good to me. It is quite clear how this will aid in implementation of the reference run comparison (#292) feature. I also ran it on some 60to30 G-case data and saw virtually identical results with current develop.

@xylar
Copy link
Collaborator Author

xylar commented Dec 30, 2017

Great, thanks @vanroekel for the quick review.

@xylar xylar removed the request for review from milenaveneziani January 1, 2018 12:06
@xylar
Copy link
Collaborator Author

xylar commented Jan 1, 2018

I'm going to go ahead an merge.

@xylar xylar merged commit b87a2e1 into MPAS-Dev:develop Jan 1, 2018
@xylar xylar deleted the clean_up_index_nino34 branch January 1, 2018 12:07
@xylar xylar mentioned this pull request Jan 26, 2018
xylar added a commit that referenced this pull request Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants