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

Fixes to setup, dependencies and CI #44

Merged
merged 6 commits into from Oct 23, 2022
Merged

Conversation

hadware
Copy link
Contributor

@hadware hadware commented Sep 29, 2022

Reference Issue

This is a PR relating to openjournals/joss-reviews#4739 (comment)

First of all, since this is our first contact, I have to say that this lib is very well made 😃 , thanks a lot for your work!
The code is very clean, the API is very clear, the tests are very exhaustive, and the documentation seems to be very complete.

The changes in this PR are somewhat nickpicky, but I think they really help making the lib more "standard".

What does this implement/fix? Explain your changes.

This fixes and "standardizes" a couple of things:

  • I move most of the constants from the spafe/version.py to the setup.py , as this was not a standard thing to do. If this choice was very opinionated (i.e., you had a very good reason to do so), you can obviously revert that.
  • I put the test and documentation dependencies in tests and docs , in the extras_require field. This is somewhat more standard, and allows the user to run pip install spafe[docs] or pip install spafe[tests] which I think is a bit cleaner. Note that running pip install spafe[anything] installs the "main" dependencies by default.
  • I left the "main" dependencies in the requirements.txt, as i think this to be pretty practical.
  • Having the ability to plot things is very good, but requiring the install of matplotlib by default is, I think, a bit bloated. I made this dependency optional, and added its install through pip install spafe[plotting]. You should probably say something about that in the install instructions.
  • I relaxed the required versions for scipy/numpy ( >= instead of == , this is very useful if spafe has to share its env with some other libraries)
  • I fixed the github actions in several ways:
    • I used a pip install instead of conda install, this makes the tests setup faster and much simpler
    • The old config didn't use the python-version matrix
    • I removed the tests for python 3.5 and 3.6 (they wouldn't be able to run anyway, the required numpy/scipy versions were not available for theses versions of python).
    • I added an action for the documentation. This couldn't run with the others actions, as you don't want a PR branch to activate a doc build.

@hbredin
Copy link
Contributor

hbredin commented Sep 30, 2022

Regarding the dependency on matplotlib, I agree with @hadware that this should be optional.

However, for some reason, running pip install spafe from a clean conda environment did NOT install matplotlib. It only installed numpy and scipy on top of spafe.

pip install spafe
Collecting spafe
  Downloading spafe-0.2.0-py3-none-any.whl (90 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 90.7/90.7 kB 1.7 MB/s eta 0:00:00
Collecting scipy>=1.4.1
  Downloading scipy-1.9.1-cp39-cp39-macosx_12_0_arm64.whl (29.9 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 29.9/29.9 MB 30.9 MB/s eta 0:00:00
Collecting numpy>=1.18.1
  Downloading numpy-1.23.3-cp39-cp39-macosx_11_0_arm64.whl (13.4 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 13.4/13.4 MB 29.7 MB/s eta 0:00:00
Installing collected packages: numpy, scipy, spafe

Does that mean that the version currently on PyPI is not on par with master?

@hadware
Copy link
Contributor Author

hadware commented Sep 30, 2022

However, for some reason, running pip install spafe from a clean conda environment did NOT install matplotlib. It only installed numpy and scipy on top of spafe.

This is because the install_requires uses these dependencies (this code is gone in my PR btw):

INSTALL_REQUIRES = (

(and thus you made me realize that matplotlib was, indeed, not required by default)

@hadware
Copy link
Contributor Author

hadware commented Oct 6, 2022

Ping @SuperKogito

@SuperKogito SuperKogito merged commit 76a2f30 into SuperKogito:master Oct 23, 2022
@SuperKogito
Copy link
Owner

First of all, thank you @hadware for these nice improvements, and I am sorry for my absence in the last weeks.

  • Your edits give more install options, which I find very useful. I was already unsure 🙈 about needing to install matplotlib, hence I think I removed it when I was setting up the conda install files.
  • The version changes are a matter of preference, but since I don't have an exact one, I will go with your suggestion.
  • Relaxing the requirements is a great idea 👏
  • Thank you so much for editing the gh-actions accordingly.

I just tested this locally, and it works fine <3 so I merging.

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

3 participants