-
Notifications
You must be signed in to change notification settings - Fork 265
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
ENH: Max CAPPI #1633
ENH: Max CAPPI #1633
Conversation
I re-added back in Max as a reviewer as well as he made some change comments on the original PR, I'm reviewing as well. |
Whether to print progress messages. Default is False. | ||
savedir : str, optional | ||
Directory where the plot will be saved. If None, the plot is not saved. | ||
show_figure : bool, optional |
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.
Same here, maybe leave it up to the user for plt.show()?
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.
Actually, it doesn't take the fig and ax arguments since its fig canvas (Main, upper, right and colorbar axes) is manually built, that is why. What do you suggest?
@@ -1110,6 +1112,46 @@ def cartopy_coastlines(self): | |||
category="physical", name="coastline", scale="10m", facecolor="none" | |||
) | |||
|
|||
def plot_maxcappi( |
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.
More curiosity, but reason for having the code in its own file and then here as well?
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.
It was a long code, didn't wanted to overcrowd the gridmapdisplay.py script
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.
What do you suggest?
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 think it would be fine being in the gridmapdisplay, or either or, we can get @mgrover1 thoughts on it as well.
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’m open to merging it back if that’s preferred. Let's see what @mgrover1 has to say!
I’ve addressed the major issues causing CI failures. The only one remaining is build-deploy-site / deploy-website. I’m not sure what that involves, so you might need to handle it. |
@syedhamidali No worries on the build doc fail, I have a fix for it in my PR |
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.
This looks great to me - I like the idea of moving a bulk of it to its own submodule, and it being used with the gridmapdisplay.
So, I have added the plot_maxcappi function to the gridmapdisplay, here is what it is supposed to do