guilgautier / DPPy

Python toolbox for sampling Determinantal Point Processes
https://dppy.readthedocs.io
MIT License
219 stars 52 forks source link

Reduce number of dependencies for DPPy #60

Closed danielecc closed 4 years ago

danielecc commented 4 years ago

DPPy currently depends by default on three "common" packages (numpy, scipy, matplotlib), and three more "uncommon" packages:

This pull decouples these three dependencies and adds them as optional, providing easy installation instruction and appropriate warnings if the user tries to use on of the extra functionalities without having the packages installed.

guillep commented 4 years ago

I'll follow the discussion about requirements.txt vs setup.py in #42.

Regarding this PR, I've only a single comment as a devil's advocate: why are there so many commits with the same comment but different content? This kind of forces people to look at the inside of the commit to see what it really contains because the message cannot be trusted...

Now, this does not necessarily mean you should fix it, I'll let @guilgautier decide on that. But it would be nice to avoid such bad practice in the future :)

danielecc commented 4 years ago

There are multiple commits with a single message because: a) most of those commits were to try out different configurations that would make Travis and ReadTheDoc happy (so commit, check build log, fix, recommit) and b) most importantly last pull request was merge-squashed, so in the master branch all those commits would be squashed into a single commit anyway. If you prefer to do other kinds of merge that preserve full history of the branch after the merge I will directly squash commits like this in my local version before committing.

guilgautier commented 4 years ago

Hi guys,

Now, this does not necessarily mean you should fix it, I'll let @guilgautier decide on that.

Given the content in the commits, basically trial and error to get Travis' green light, the duplicate commit names are not crucial to follow the course of action

Nevertheless, I agree with @guillep

But it would be nice to avoid such bad practice in the future :)

guilgautier commented 4 years ago

However, given the current GitHub display I don't see that the commits with duplicate names have been squashed into one. Maybe you've done the squash locally but didn't git push --force remotely @danielecc ?

danielecc commented 4 years ago

I did not squash locally. What I meant is that instead of clicking "merge pull request" to merge in master, we can simply use "squash and merge" (from the drop down menu).

The single commit message can look something like

Removed dependencies from setup.py, refactored to deal with changes

Moved imports of optional dependencies inside classes and added warnings if missing Updated docs and main readme file to reflect this change

guilgautier commented 4 years ago

Must be a new GitHub feature, let's go for the merge!