-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
DFT PR - 7 #3974
DFT PR - 7 #3974
Conversation
@GreatRSingh there seems to be some installation failure on windows, could you please resolve it |
requirements/env_common_3_8.yml
Outdated
@@ -27,3 +27,4 @@ dependencies: | |||
- xgboost | |||
- gensim # used by mol2vec | |||
- tensorboard | |||
- basis-set-exchange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be a required dependency for anyone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was removing it but saw that it was used in many places, and removing it will require adding try except everywhere.
So i have shifted this dependency from common to torch env.
@shreyasvinaya I have discussed about it with bharath sir, it should not be a big issue as most people use deepchem on linux or WSL. I will look into fixing this issue some time later. Will have to check if the dependencies can work on windows or not. |
016b62d
to
da00434
Compare
try: | ||
import basis_set_exchange as bse | ||
except Exception as e: | ||
print(f'basis-set-exchange Not Found: {e}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please swap this to a logging statement. We should not have prints for errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some doctest failures related to this PR that need to be fixed. There are also other unrelated failures @GreatRSingh will fix those in a separate PR
acb5a4c
to
afc9ddf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* forward * 3.8 * added to docs * shift from common to torch env * mypy fixes * doctest fixes
Description
Fix #(issue)
Type of change
Please check the option that is related to your PR.
Checklist
yapf -i <modified file>
and check no errors (yapf version must be 0.32.0)mypy -p deepchem
and check no errorsflake8 <modified file> --count
and check no errorspython -m doctest <modified file>
and check no errors