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

Use setup.cfg file instead of setup.py #87

Merged
merged 9 commits into from
Feb 5, 2021
Merged

Use setup.cfg file instead of setup.py #87

merged 9 commits into from
Feb 5, 2021

Conversation

yannikschaelte
Copy link
Member

@yannikschaelte yannikschaelte commented Feb 4, 2021

  • No conceptual difference, just kind of "cooler" that the package definition is not in python but pure text.
  • Added a bit more meta info (classifiers, URLs, ...)
  • I set the minimum python version to 3.7, as e.g. scipy+amici also no longer support 3.6. Opinions? If 3.7 is set as minimum, a few docs need to be updated.
  • Also reduced the tests to 3.7+3.9 (min+max). I think that should be better than testing all intermediate versions

TBD: I think the dependencies should be tidied up afterwards, e.g. the test requirement is not used in the actual tests yet.

@codecov-io
Copy link

codecov-io commented Feb 4, 2021

Codecov Report

Merging #87 (d82306d) into develop (3c45897) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #87   +/-   ##
========================================
  Coverage    77.85%   77.85%           
========================================
  Files            5        5           
  Lines          429      429           
========================================
  Hits           334      334           
  Misses          95       95           

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 3c45897...d82306d. Read the comment docs.

@yannikschaelte yannikschaelte self-assigned this Feb 4, 2021
@yannikschaelte yannikschaelte mentioned this pull request Feb 4, 2021
Merged
@jvanhoefer
Copy link
Member

Is there any benefit from requiring python > 3.7 or don't we just exclude users for no obvious reason?

setup.cfg Outdated
[options]
install_requires =
numpy >= 1.19.4
scipy >= 1.6.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scipy is actually not used in the public API, but just in the examples...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to know, my mistake! will move it back to the examples.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it should be exactly as before, also again including python 3.6.

@yannikschaelte
Copy link
Member Author

Is there any benefit from requiring python > 3.7 or don't we just exclude users for no obvious reason?

I think one can support 3.6, my take was rather "ok, when even the very basic modules like scipy drop 3.6, hardly any user will use 3.6 any more soon, so not worth taking the trouble here". But as some systems still run on 3.6, we can include it here. In that case the tests need to be subdivided though, e.g. if we add tests using AMICI, those need to be restricted to 3.7+.

Most typical applications with PEtab or AMICI or an up-to-date scipy will only work for 3.7+ (and soon 3.8+, which introduces some nice language features). Usually, we only support the last two versions in our packages. If it is only about the conversion YAML -> SBML, that should work with 3.6.

In summary: Both works. Which do you prefer?

@yannikschaelte yannikschaelte merged commit 64211f9 into develop Feb 5, 2021
@yannikschaelte yannikschaelte deleted the fixes_ys branch February 5, 2021 11:40
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.

3 participants