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

REF: make ipywidgets optional dependency #132

Merged
merged 11 commits into from
Jun 30, 2021
Merged

REF: make ipywidgets optional dependency #132

merged 11 commits into from
Jun 30, 2021

Conversation

martinfleis
Copy link
Member

Fixes #130

CI is failing on some numba stuff I did not try to debug.

Btw, we should really try to do #124.

@jGaboardi
Copy link
Member

jGaboardi commented Jun 26, 2021

Btw, we should really try to do #124.

Agreed 100%, though I don't want to step on @slumnitz's toes and make changes without her approval.

@jGaboardi
Copy link
Member

Seems like the first thing to do would be to switch to main as the default branch and remove master.

@slumnitz
Copy link
Member

@martinfleis thank you or taking care of this! Could you add a test as well?

@martinfleis
Copy link
Member Author

martinfleis commented Jun 28, 2021

I wanted to but we would need to revise CI to test in the environment with and without ipywidgets which doesn't seem to be possible now.

@slumnitz
Copy link
Member

@martinfleis ok, most new CI testing happened in https://github.com/pysal/splot/tree/main I am currently not sure anymore why this branch failed. I unfortunately do not have much time this week. I think I could try giving it a go Wednesday during the day, and do testing of this later.

@martinfleis
Copy link
Member Author

I will add the test here and run it locally. Then we can have a go at #124 with @jGaboardi with your permission and ensure that it is properly tested on CI. That way we can merge this and release it before the feature freeze.

@jGaboardi
Copy link
Member

In order to complete #124 we will need full admin privileges (for Secrets, etc.).

@martinfleis
Copy link
Member Author

It may actually be overly complicated to do this test withoutpytest (read I don't know how to do it with nose). Can we make a note in #124 or open an issue and add this test later?

@sjsrey
Copy link
Member

sjsrey commented Jun 28, 2021

Just a note that yesterday we removed support for 3.6 and added 3.9 support for libpysal. So if 3.6 is failing in splot, this is likely one reason.

@sjsrey
Copy link
Member

sjsrey commented Jun 28, 2021

In order to complete #124 we will need full admin privileges (for Secrets, etc.).

@jGaboardi and @martinfleis now have admin rights.

@martinfleis
Copy link
Member Author

CI is failing since March 1 (https://travis-ci.org/github/pysal/splot/builds/760938486).

@slumnitz
Copy link
Member

slumnitz commented Jun 30, 2021

@martinfleis & @jGaboardi we used to start fixing all bugs on the main branch, not the master branch anymore and got that one ready including a new testing procedure. The main branch currently seems to pass, however I am unsure if this is correct. Maybe @jGaboardi you could confirm? if so, we should make main the new default and point all new pr's towards main instead of master.

@martinfleis
Copy link
Member Author

I didn't know we have both master and main (confusing). Should I target this PR to main then?

@slumnitz
Copy link
Member

@martinfleis we were in the middle of migrating to main and also of during this process changing the testing procedure. However as far as I remember the testing did not work and we could not figure out why. Help is much appreciated. Maybe target it towards main and we see if testing there runs through.

@martinfleis martinfleis changed the base branch from master to main June 30, 2021 09:29
@martinfleis
Copy link
Member Author

let me clean the PR, changing base caused a mess

@martinfleis
Copy link
Member Author

cool, it seems that the new CI is up and running! I'll add the test then and remove ipywidgets from one of the env files to check it.

@martinfleis
Copy link
Member Author

Though it doesn't seem to work...

@slumnitz
Copy link
Member

slumnitz commented Jun 30, 2021

I think it fails due to libpysal not working with 3.6. Can you add the correct python versions within this PR? Just dropping 3.6 adding 3.9.

@slumnitz
Copy link
Member

ok now, for some reason it is testing _viz_bokeh.py which is not user exposed and by now likely deprecated (a remain of GSOC 2018 remaining for completeness and potentially for future PR's) is there a way to define not to test this functionality?

@martinfleis
Copy link
Member Author

I added skips for bokeh tests.

@martinfleis
Copy link
Member Author

And test of ipywidgets import error.

@codecov-commenter
Copy link

Codecov Report

Merging #132 (686d138) into main (077a5b8) will decrease coverage by 12.20%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #132       +/-   ##
===========================================
- Coverage   98.15%   85.94%   -12.21%     
===========================================
  Files          18       18               
  Lines        1195     1217       +22     
===========================================
- Hits         1173     1046      -127     
- Misses         22      171      +149     
Impacted Files Coverage Δ
splot/tests/test_viz_bokeh.py 33.33% <31.57%> (-66.67%) ⬇️
splot/_viz_giddy_mpl.py 94.70% <100.00%> (+0.07%) ⬆️
splot/tests/test_viz_giddy_mpl.py 100.00% <100.00%> (ø)
splot/_viz_bokeh.py 16.50% <0.00%> (-81.56%) ⬇️
splot/_viz_utils.py 56.56% <0.00%> (-37.38%) ⬇️

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 077a5b8...686d138. Read the comment docs.

@jGaboardi
Copy link
Member

I think this is good to merge :shipit:

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.

ipywidgets dependency
5 participants