Skip to content
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

Extra dims slider #417

Merged
merged 17 commits into from Jul 11, 2019
Merged

Extra dims slider #417

merged 17 commits into from Jul 11, 2019

Conversation

doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jul 3, 2019

in a notebook, after plotting, a slider appears for any extra dimensions plotted, moving the slider around update the picture.

Needed to fix a few things in VTK's backend:

update wouldn't work for vectors, isofill where using wrong contours, etc...

I think this is ready to go. Please ignore zillions of if debug: for now, I need them until we are sure it works.

Copy link
Collaborator

@scottwittenburg scottwittenburg left a comment

I've added a few comments here and there where I could grasp what was going on. In general, I'm not sure I understand the motivation for this change. Is it all so that you can have sliders on an ipywidget in the jupyter lab notebooks?

Regardless of me understanding the motivation, I didn't see any obvious problems. So if it's passing all the tests, I guess it should be fine. I noticed you added a file under uvcdat-testdata though, you might want to remove that before merging this.


class TestVCSBoxfillPickFrame(basevcstest.VCSBaseTest):
def testBoxfillPickFrame(self):
f = cdms2.open(os.path.join(cdat_info.get_sampledata_path(), "ta_ncep_87-6-88-4.nc"))
Copy link
Collaborator

@scottwittenburg scottwittenburg Jul 3, 2019

Choose a reason for hiding this comment

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

Should the class and method name be changed to match the filename (i.e. something with isofill)?

Copy link
Contributor Author

@doutriaux1 doutriaux1 Jul 3, 2019

Choose a reason for hiding this comment

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

yep... you got me copy/pasting :) Will fix

Copy link
Contributor Author

@doutriaux1 doutriaux1 Jul 3, 2019

Choose a reason for hiding this comment

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

Actually these files are no longer need i created a more general test, will remove it.


try:
import IPython.display
import ipywidgets
Copy link
Collaborator

@scottwittenburg scottwittenburg Jul 3, 2019

Choose a reason for hiding this comment

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

My naive thought would be you set some variable here which you can later use to determine whether you have widgets or not. Otherwise, do you just assume the import worked later on?

if debug:
IPython.display.display(self._parent._display_target.out)
IPython.display.display(*widgets)
IPython.display.display(IPythonDisplay(st))
Copy link
Collaborator

@scottwittenburg scottwittenburg Jul 3, 2019

Choose a reason for hiding this comment

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

Does this mean vcs now depends on IPython? Or maybe it always did, and I just didn't realize it?

Copy link
Contributor Author

@doutriaux1 doutriaux1 Jul 3, 2019

Choose a reason for hiding this comment

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

it's inside a try that starts with import IPython. So no it does not depend on it but takes advantage of it if present.

Copy link
Contributor Author

@doutriaux1 doutriaux1 Jul 3, 2019

Choose a reason for hiding this comment

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

No you're right it's been moved to its own section but only called from inside the try. I'll still use the variable way you suggested to prevent accidental use.

try:
import IPython.display
# import cdat_notebook
Copy link
Collaborator

@scottwittenburg scottwittenburg Jul 3, 2019

Choose a reason for hiding this comment

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

I guess there was already a dependency on IPython...

@scottwittenburg
Copy link
Collaborator

scottwittenburg commented Jul 3, 2019

Looking back at my review, the mention of adding a baseline image in the uvcdat-testdata might have been kind of hidden in my initial review comments.

Don't forget to remove that 😄

@coveralls
Copy link

coveralls commented Jul 8, 2019

Coverage Status

Coverage remained the same at ?% when pulling 1e5feb3 on extra_dims_slider into 266121b on master.

@doutriaux1
Copy link
Contributor Author

doutriaux1 commented Jul 8, 2019

@sterlingbaldwin @downiec please try it in your envs. If it works for you, let's merge in.

@doutriaux1 doutriaux1 merged commit ff40414 into master Jul 11, 2019
@doutriaux1 doutriaux1 deleted the extra_dims_slider branch Jul 11, 2019
@sterlingbaldwin
Copy link

sterlingbaldwin commented Jul 23, 2019

Works for me!

@downiec downiec added this to the 8.2 milestone Oct 21, 2019
@downiec downiec removed this from the 8.2 milestone Jul 27, 2020
@downiec downiec added this to the 8.2.1 milestone Jul 27, 2020
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.

None yet

5 participants