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

Fix widget reset bugs #212

Merged
merged 6 commits into from
Jun 4, 2019
Merged

Conversation

jsub1
Copy link

@jsub1 jsub1 commented May 29, 2019

Fixes a couple of issues I found in the reset method:

  1. Only half of the layers were removed when resetting the widget.
  2. An exception was thrown when resetting the jupyter widget because we were also resetting the inherited DOMWidget traits.

Jeffrey SubbaRao added 2 commits May 27, 2019 21:50
… of all of them. The culprit was using a iterator loop while removing elements in the list
…ting the WWT traits to default values in the method.
@astrofrog
Copy link
Member

The issues in the tests are real - I think the problem is that foreground and background aren't traits that have the wwt tag (deliberately). Is that change really needed? What traits are being incorrectly reset?

@jsub1
Copy link
Author

jsub1 commented May 29, 2019

There are about 10 different traits inherited from the DOM widget or one of its ancestors where trying to restore them to a default value will cause an exception to be thrown (_dom_classes, comm, etc.).

We could iterate over and reset the traits that are specifically defined in the base widget class, instead of all of the traits that a widget has.

@astrofrog
Copy link
Member

We could iterate over and reset the traits that are specifically defined in the base widget class, instead of all of the traits that a widget has.

Yes, though we should make sure we are sub-class friendly too in case anyone ever defines traits in a subclass.

@jsub1
Copy link
Author

jsub1 commented May 31, 2019

We could iterate over and reset the traits that are specifically defined in the base widget class, instead of all of the traits that a widget has.

Yes, though we should make sure we are sub-class friendly too in case anyone ever defines traits in a subclass.

The most general/transparent solution I can think of is to add a wwt_reset tag to the traits that we want to reset.
Another solution would be to loop over both BaseWWTWidget.class_traits() and self.class_own_traits(), but that wouldn't allow as much control as the above solution (and would break if we ever add a widget that doesn't inherit directly from the BaseWWTWidget).

@astrofrog
Copy link
Member

Adding wwt_reset works for me!

@pkgw
Copy link
Contributor

pkgw commented Jun 3, 2019

That sounds like a good solution to me too!

AppVeyor CI failure is an issue of antialiasing and color intensity (opacity?) of overdrawn lines in the Python 3.6 build. Presumably unrelated to this PR.

Travis CI failures are due to an error in make -C docs linkcheck with what looks like a transient issue with GitBook, or possibly a too-short timeout because the problematic URL (https://worldwidetelescope.gitbook.io/layer-control-reference/lcapicommands#table-of-properties) does take a really long time to load.

@astrofrog OK to go ahead and merge?

@jsub1
Copy link
Author

jsub1 commented Jun 3, 2019

One of the Travis failure (3.5) is due to the incompatibility of pytest 4.5 and pytest-remotedata 0.3.0 (Relevant issue: astropy/pytest-remotedata#37). Can we make sure it grabs remotedata 0.3.1 or later going forward?

@pkgw
Copy link
Contributor

pkgw commented Jun 3, 2019

Since I went to the trouble of generating them, here are the images from the AppVeyor CI failure.

expected

expected

actual

actual

@pkgw
Copy link
Contributor

pkgw commented Jun 3, 2019

@jsub1 Hmm, not sure why it's pulling in the older version, but let's try forcing >= 0.3.1 and see what happens ... I just added a commit to your branch doing so.

@pkgw
Copy link
Contributor

pkgw commented Jun 3, 2019

Hmm, on Linux/Py35 it says that it's using remotedata 0.3.1 but the errors are still happening.

Same general deal as in the previous commit.
@pkgw
Copy link
Contributor

pkgw commented Jun 3, 2019

Oh, looks like we need pytest-cov >= 2.6.1 as well. Commit pushed.

@codecov
Copy link

codecov bot commented Jun 3, 2019

Codecov Report

Merging #212 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #212      +/-   ##
==========================================
+ Coverage   57.74%   57.76%   +0.02%     
==========================================
  Files          24       24              
  Lines        1692     1693       +1     
==========================================
+ Hits          977      978       +1     
  Misses        715      715
Impacted Files Coverage Δ
pywwt/core.py 69.72% <100%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f914b9...d8f8e60. Read the comment docs.

@pkgw
Copy link
Contributor

pkgw commented Jun 3, 2019

Aha! The Travis CI doesn't use conda-forge to source packages, while AppVeyor does. I think these various issues have been due to getting older versions of packages from the more conservative Anaconda defaults channel.

Since AppVeyor uses conda-forge and we've just added pywwt to conda-forge, I've tried adding it to the Travis configuration here. Hopefully this will make these issues go away. And in recent runs, the GitBook issue has disappeared, meaning that the only CI problem left should be the AppVeyor/Py36 issue. I hope.

@pkgw
Copy link
Contributor

pkgw commented Jun 3, 2019

OK! The only Travis issue was a transient link-checking issue, so we've at least made progress on the CI front.

@astrofrog look OK?

@pkgw pkgw merged commit 3f9eb34 into WorldWideTelescope:master Jun 4, 2019
astrofrog pushed a commit that referenced this pull request Jun 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants