-
Notifications
You must be signed in to change notification settings - Fork 31
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
Refactoring of vrfy - transferred bulk of plotting to sub-script #538
Conversation
Automated Global-Workflow GDASApp Testing Results:
|
Automated Global-Workflow GDASApp Testing Results:
|
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.
Still a bit of a mess ... Which I created!
1 - For the state figures, we have 4 cases that are generating similar figures, the difference being the bounds, and input files ...
Instead of splitting into plot_analysis
and plot_increment
, split into:
plot_increment
plot_background
plot_analysis
plot_bkgerr
Ideally, we should create a simple ViewState
class that would look something like this:
class ViewState:
def __init__(self, config_dict):
self.config = config_dict
self.stuff = config_dict['stuff']
...
# Where config contains the bounds, filename and whatever else is needed to plot stuff
def plot(self):
# Loop over variables, slices (horiz and vertical) and projections ... and whatever else is needed
Now instead of having 4 separate function, you have one class that you "instantiate" 4 times with different configurations. In the ex global script, you'd do something like this:
config= ...
viewIncr = ViewState(config)
viewIncr.plot()
config= ...
viewBkg = ViewState(config)
viewBkg.plot()
config= ...
viewAna = ViewState(config)
viewAna.plot()
config= ...
viewBkgErr = ViewState(config)
viewBkgErr.plot()
That should remove quite a bit of code and make things a bit more readable.
Hope that makes sense, happy to chat if not.
2 - Move the soca_vrfy.py
into ush/soca
@guillaumevernieres This is a minor thing, but should |
If we're the only one using it, I would say yes. |
Automated Global-Workflow GDASApp Testing Results:
|
Automated Global-Workflow GDASApp Testing Results:
|
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 for doing this @AndrewEichmann-NOAA . Looks good. Just one comment related to variable naming convention.
Now python coding style is failing on |
It looks like it's a simple fix @AndrewEichmann-NOAA . Maybe |
Automated Global-Workflow GDASApp Testing Results:
|
@guillaumevernieres This currently passes soca ctests on Hera |
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.
👍
No functional changes, just moved plotting-specific code from vrfy script to script in ush. Also cleaned up some superfluous variables.