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

Prep v0.6.0 #226

Merged
merged 9 commits into from
Nov 6, 2024
Merged

Prep v0.6.0 #226

merged 9 commits into from
Nov 6, 2024

Conversation

diogomart
Copy link
Contributor

Updated version strings to v0.6.0
Moved scripts to meeko/cli, per conda-forge requirements and already implemented in v0.5.1
Removed script mk_get_openff_epsilon_sigma.py

Copy link
Contributor

@rwxayheee rwxayheee left a comment

Choose a reason for hiding this comment

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

This looks good, thanks!

@rwxayheee
Copy link
Contributor

rwxayheee commented Nov 6, 2024

Would you want to put dependencies in setup.py install_requires=, or do you prefer them to stay in installation guide?
I'm fine with either way
The template making function introduces a new dependency (gemmi)

@diogomart
Copy link
Contributor Author

I'm including as you suggest unless @nbruciaferri objects due to conflicts with conda.
Advantage of including is that a local pip install from source pip install . will automatically fetch the dependencies from PyPI. Numpy is already included, we are missing rdkit, scipy, and gemmi. Not including prody because it brings biopython, and pyparsing, and downgrades numpy, and it is possible that users have a more complex environment situation if they are installing from source. Users can always manually install prody. Also making pandas an optional dependency, only used to convert interaction analysis to a dataframe. This function is not used by ringtail.

@rwxayheee
Copy link
Contributor

rwxayheee commented Nov 6, 2024

Thanks! Ringtail also needs pandas although there's no explicit install_requires= in setup.py. I'm completely fine either way

@diogomart
Copy link
Contributor Author

I think a pip installation from PyPI should install dependencies. At least that's what I'm seeing for popular packages. For example, pip install scipy installs numpy. And pandas and matplotlib install a bunch of stuff. The issue for ringtail might be that chemicalite is not on PyPI, only on conda-forge, but @maylinnp might drop that dependency in the future. forlilab/Ringtail#58

@nbruciaferri
Copy link
Contributor

As long as we already checked that the packages in the install_requires= are working and available I'm fine with it.

Copy link
Contributor

@nbruciaferri nbruciaferri left a comment

Choose a reason for hiding this comment

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

Looks good to me, ready to merge!

@diogomart diogomart merged commit 1a2de91 into develop Nov 6, 2024
1 check passed
@diogomart diogomart deleted the prep_v0.6.0 branch November 6, 2024 22:33
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.

5 participants