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

Make pandas and mpl dependencies for optional features #141

Merged
merged 5 commits into from
Jan 13, 2022

Conversation

maxscheurer
Copy link
Member

@maxscheurer maxscheurer commented Jan 12, 2022

mpl is used in two functions, pandas in one function, both could be considered as optional features (see #140).
This PR removes the two modules as mandatory dependencies, making adcc a bit more lightweight. 🚀

A new decorator for the functions requiring the modules checks upon call whether the modules are found on the system or not and gives instructions on how to obtain them.

Unrelated to that, I have fixed all broken example python scripts in examples.

@maxscheurer maxscheurer force-pushed the optional_analysis_deps branch 2 times, most recently from 6877af7 to 73b4b4d Compare January 12, 2022 10:51
@mfherbst
Copy link
Member

mfherbst commented Jan 12, 2022

Good idea! It's always good to keep things as lightweight as possible.

Could you also add this to the docs (installation instruction and the tutorial guide we have). People might expect the analysis features to "just work" only to be presented with an error when they try it ...

@maxscheurer
Copy link
Member Author

maxscheurer commented Jan 12, 2022

Yes, I was going to add docs anyways, but was waiting for your feedback on the idea in general 👍

@maxscheurer
Copy link
Member Author

Docs added. Note that I won't update the local conda recipes anymore, they will hopefully be removed soon...

Copy link
Member

@mfherbst mfherbst left a comment

Choose a reason for hiding this comment

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

A few more nits about the docs. In DFTK we actually started running the (small and fast) examples in CI to ensure the code is not suddenly broken. Maybe that's something we should think about as well?

docs/installation.rst Outdated Show resolved Hide resolved

Optional dependencies for analysis features
-------------------------------------------

Copy link
Member

Choose a reason for hiding this comment

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

From my experience users rarely read such a block text in the docs. I think it's wise to first show a list of "optional packages" along with a code block listing the conda or pip command to install them. You can then add the more detailed info in a paragraph afterwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are right. I removed the large block text and replaced it with a concise list, looks much better now. I'll have a look at the docs from build artifacts and will merge the PR if everything looks fine.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to be.

@maxscheurer
Copy link
Member Author

I noticed that sphinx does not process some files, and as such they do not appear in the docs 😠 Wonder why we didn't notice it before...


Optional dependencies for analysis features
-------------------------------------------

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to be.

@mfherbst
Copy link
Member

Wonder why we didn't notice it before...

Probably because no-one checked ...

@maxscheurer maxscheurer mentioned this pull request Jan 13, 2022
4 tasks
@maxscheurer
Copy link
Member Author

Now the docs are looking good again 😄

@maxscheurer maxscheurer merged commit f3a7aad into master Jan 13, 2022
@maxscheurer maxscheurer deleted the optional_analysis_deps branch January 13, 2022 11:54
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

2 participants