-
Notifications
You must be signed in to change notification settings - Fork 12
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 for Hvplot batch plotting #158
Conversation
I probably wont have time to review before furlough. But just one quick spot and comment. Since the YAMLs do have to change I think we might as well make the backend a required argument. I.e. add EMCpy to all the test YAMLs. |
Ditto for me with what Dan said :-( here's hoping I see GitHub again soon |
Could you update the requirements.txt to include these new packages? I think we should have three requirements packages:
The first one used by github stays as is. The second one should be the same as the first one but with >= instead of == and not include the things needed to build the docs. The third _emc would be the same as the current requirements.txt. I.e. not have seaborn, bokeh, hvplot etc. Would that arrangement work for you @CoryMartin-NOAA? Would it be possible to create a situation where if the chosen backend is hvplot but this package is not available we get a sensible error? try:
import hvplot
except ImportError:
logger.abort("The hvplot backend is not available since hvplot is not in the environment.") |
Yes, @danholdaway that sounds perfect. |
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.
Just a couple of very minor comments. Overall this looks great. I like that you have have essentially the same YAML with just one key different and can make the plots using EMCpy and HVplot. Thanks for working on this.
One other thing. Can you make a minor version bump in setup.py to 1.6.0? Then once we have a fully working hvPLot backend perhaps we make it eva 2.0 since that would be a significant new feature. |
I just pushed a little clean up to the requirements. There were still some lingering things in setup.py that are replaced by what's in all the requirements.txt files. I figure we should minimize the number of places we have these dependencies. I also added a requirements file that is used when installing with spack-stack 1.4 |
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.
You've got a swap file in here.
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.
A few extraneous swapfiles merged in, but beyond that (and figuring out the Actions failure) this looks good to me.
b644d46
to
9214290
Compare
This reverts commit 9214290.
@danholdaway I just pushed a branch that deleted your changes, and I tried to revert but I don't see files like the spack stack requirements file anymore. Can you revert back to the changes you made and then I can try again with the review changes? |
…internal/eva into feature/bokeh_batch_plots
I think they are still there when I look at https://github.com/JCSDA-internal/eva/tree/feature/bokeh_batch_plots |
I think something is going on on my end because I'm not seeing your commit in my commit history. Do you mind sharing the commit id? |
Can you
Can you try doing a git pull? Otherwise perhaps just reclone the repo? I'm at commit d54420c |
Oh maybe I hadn't pushed everything I had locally. I just did a push and something went. |
I don't know why, but whatever you did made all of your commits reappear in my commit history (like the one from a few hours ago). Odd github behavior, but we're back on track now |
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.
Looks like the tests pass now with that requirements change, but we still have loitering swapfiles committed.
Yep, those will be taken care of in just a moment |
## Description This PR expands upon the hvplot backend introduced in #158. All other diagnostic types are reformatted to have a base class and child classes for each backend. Each base class includes two methods: `data_prep` and `configure_plot`. `configure_plot` is a virtual method that is resolved once a child class for either emcpy or hvplot is instantiated. `figure_driver` is updated to reflect these changes
Description
We would like to use eva to create batches of hvplot figures for future web development work. In order to do this, the figure driver needs to be generalized to take in multiple backends. Then, the emcpy and hvplot backends would be selected based on the
plotting_backend
field in the config yaml. A handler was created to direct each backend to it's associated CreatePlot and CreateFigure classes.Please note the new test yaml:
testHvplotIodaObsSpaceAmsuaN19.yaml
. The hvplot backend will expect the same yaml configurations that emcpy does.Currently, only the scatter plot works for the hvplot backend. We've decided that it would be best to get feedback on the restructuring before we moved on with further hvplot development considering there are a lot of changes.
The new directory structure has two main directories under plotting:
batch
andinteractive
. Under thebatch
directory will be a base directory and backend directories that will inherit from the base directory. In the future, all diagnostics will inherit data prepping/processing from the base diagnostic classes.