Navigation Menu

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

mutable arguments removed #167

Merged
merged 5 commits into from Apr 24, 2019
Merged

Conversation

cmorph1
Copy link

@cmorph1 cmorph1 commented Apr 8, 2019

I have removed the mutable arguments, I haven't touched the Color_scale argument.

@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #167 into master will increase coverage by 2.53%.
The diff coverage is 53.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
+ Coverage   76.52%   79.05%   +2.53%     
==========================================
  Files          10       10              
  Lines         707      721      +14     
  Branches      141      148       +7     
==========================================
+ Hits          541      570      +29     
+ Misses        140      120      -20     
- Partials       26       31       +5
Impacted Files Coverage Δ
kmapper/plotlyviz.py 42.76% <50%> (+15.29%) ⬆️
kmapper/kmapper.py 88.88% <66.66%> (-0.23%) ⬇️

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 ebe6bda...6dd07ed. Read the comment docs.

@sauln
Copy link
Member

sauln commented Apr 8, 2019

Looks good, thanks for doing this!

It looks like there are no tests that use the lens_names property. Would you mind adding one so we can be sure it is working correctly?

Thank you!

@cmorph1
Copy link
Author

cmorph1 commented Apr 8, 2019

I get the feeling I am going to sound stupid, but how do you add a test?

@sauln
Copy link
Member

sauln commented Apr 8, 2019

not at all! learning about tests is always a hard hurdle.

In the source directory, you can install the test requirements into your environment with the command

pip install -e ".[testing]"

Then, you can run the test suite with the command

pytest test

You should see a summary of all the test runs showing that they've passed.

Then you can add a new test to the file test/test_mapper.py and try running the tests again.

We use pytest for all our tests, you can read more about it on their docs site and look at some of the other tests to see how we structure them.

Let me know if you need anything else!

@cmorph1
Copy link
Author

cmorph1 commented Apr 8, 2019

Thanks I'll give it a go

@cmorph1
Copy link
Author

cmorph1 commented Apr 8, 2019

When I am trying to run pytest test, I'm being told I need to:
pip install python-igraph plotly ipywidgets
But that fails when I try to install it. I'm not sure what my next move should be

@sauln
Copy link
Member

sauln commented Apr 8, 2019

Did running

pip install -e ".[testing]"

result in errors? That command should install those 3 packages. If that did not create errors, you might have a problem with your environments being crossed. Are you using virtual environments?

Compare the output of which pytest and which python, making sure they are both pointing to similar places.

@cmorph1
Copy link
Author

cmorph1 commented Apr 8, 2019

I got this error:
'Cannot find the C core of igraph on this system using pkg-config'
I've had workspace issues on this laptop with the virtual environments before. I've pip uninstalled everything on there and try creating another venv tomorrow. Thanks for your help!

@sauln
Copy link
Member

sauln commented Apr 8, 2019

Ahh, if you are on Windows you should follow the instructions here: https://igraph.org/python/

Sometimes setting igraph up can be a pain point.

If you still can't get it working, you can specify just running the test file you're extending with the command

pytest test/test_mapper.py

@cmorph1
Copy link
Author

cmorph1 commented Apr 9, 2019

Thanks for the help Nathaniel, I'll have a go at this tonight.

@cmorph1
Copy link
Author

cmorph1 commented Apr 9, 2019

I think I have done it. Let me know if I need to do anything more.

@sauln
Copy link
Member

sauln commented Apr 9, 2019

It looks like you've added a test, but it still has the body of the test you copied above. Maybe you need to make one more commit with the changes to the test?

@cmorph1
Copy link
Author

cmorph1 commented Apr 10, 2019

Sorry I'll change this tonight.

@cmorph1
Copy link
Author

cmorph1 commented Apr 10, 2019

The test is failing, but I think its down to how I've built it as I am getting this error:
"> if not len(graph["nodes"]) > 0:
E IndexError: only integers, slices (:), ellipsis (...), numpy.newaxis (None) and integer or boolean arrays are valid indices"
I may need to install i-graph and get a better understanding of how it works.

@sauln
Copy link
Member

sauln commented Apr 10, 2019

igraph is a dependency for some of the visualization tools we use, but you shouldn't have to use it other than having it installed.

It's hard to diagnose the issue without the code. Feel free to push what you have no and I can make edits if need be.

Thanks!

@cmorph1
Copy link
Author

cmorph1 commented Apr 11, 2019

I'll have one last go at it tonight and if I can't work it out I'll push for you to review, thanks Nathaniel

@cmorph1
Copy link
Author

cmorph1 commented Apr 11, 2019

Hi Nathaniel,
Unfortunately I'm lost. I tried to install the extra packages and had the following error:
DeprecationWarning: To avoid name collision with the igraph project, this visualization library has been renamed to 'jgraph'. Please upgrade when convenient.

When I try to run test_mapper I get this error:
IndexError: only integers, slices (:), ellipsis (...), numpy.newaxis (None) and integer or boolean arrays are valid indices

I will push my test through to you. It would be a great help if you could explain what I have done wrong so I can learn from it.

@sauln sauln changed the title mutable arguments removed (not including color_default dict) mutable arguments removed Apr 24, 2019
@sauln sauln mentioned this pull request Apr 24, 2019
@sauln sauln merged commit d4ed39f into scikit-tda:master Apr 24, 2019
sauln pushed a commit that referenced this pull request May 17, 2019
Merge: ebe6bda 6dd07ed
Author: Nathaniel Saul <nat@saulgill.com>

    Merge pull request #167 from cmorph1/remove-mutable-arguments
sauln pushed a commit that referenced this pull request May 17, 2019
Merge: ebe6bda 6dd07ed
Author: Nathaniel Saul <nat@saulgill.com>

    Merge pull request #167 from cmorph1/remove-mutable-arguments
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.

None yet

2 participants