ContextLab / hypertools

A Python toolbox for gaining geometric insights into high-dimensional data
http://hypertools.readthedocs.io/en/latest/
MIT License
1.83k stars 160 forks source link

Small load() refactor #171

Closed Aylr closed 6 years ago

Aylr commented 6 years ago

I removed some duplicate code in the load() function and added a helpful error message if a user typos one of the example files like I did when trying the library.

jeremymanning commented 6 years ago

Hi @Aylr, thanks for your submission, and I like most of your changes! It looks like our tests are throwing an error when using your new load function. You can use the example scripts in the test folder to ensure your code passes our checks (we test on python 2.7x and 3.4, 3.5, and 3.6).

I took a quick skim through your code, and I think the issue might be in your return statement-- you're directly returning the result of pickel.loads, whereas the prior version allowed the user to return an analyzed version of the dataset by passing the loaded data through analyze.

andrewheusser commented 6 years ago

@Aylr , @jeremymanning - I think the issue is in the if data: evaluation. The travis error is:

tests/test_align.py:11: in <module>
    data1 = load('spiral')
hypertools/tools/load.py:81: in load
    if data:
E   ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

I think if you change it to:

if data is not None:

it should pass the tests

jeremymanning commented 6 years ago

Ah, yes, I think @andrewheusser is right. I now see that you do pass data through analyze, so my earlier comment was wrong (that return statement was for _load_stream, not the main load function...serves me right for viewing the code in the GitHub "code differences" browser!). So if you fix line 81 we should be good.

Aylr commented 6 years ago

Is there something I can do to improve or change this PR to consider merging? Did I miss work elsewhere?

jeremymanning commented 6 years ago

Hi @Aylr,

Once you fix that one line (81 in load.py), just submit another request and we'll merge! (Assuming the tests pass)

andrewheusser commented 6 years ago

hey @Aylr - if you change line 81 from if data: to if data is not None: the code should pass our tests and then i'll merge, thanks!

Aylr commented 6 years ago

I apologize it took me so long to respond! Thanks for your guidance and patience. The line 81 fix is in.

jeremymanning commented 6 years ago

@Aylr this looks great to me-- merging! 🎉

Inspired by this pull request, I've added a new issue with other potential extensions of load.