-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add farhadh/zasdfgbnm changes #21
Add farhadh/zasdfgbnm changes #21
Conversation
* Specify dtype for species For some reason, sometimes species are constructed as int tensors instead of long tensors, this is wrong and should be fixed * Update __init__.py
* skip downloading into site-packages * add placeholder files * add placeholder files * dont check filesize in local_dir * fix shutil erro * use distutils instead of shutils
* Update JCIM issue * Add cover and retrigger build
* Distribute source and weel * trigger * fix
Are you saying that after you run the test, the placeholders in the source repo will be changed to a real file? As a workaround, does it make sense to install torchani by |
|
@zasdfgbnm Looks like this can be avoided by running git update-index Yes, I noticed the issue with Final when doing some stuff with the ONNX support. Final is actually not needed right? it supposedly only adds some performance improvements for JIT |
We can either remove Final altogether, remove support for 3.6, or put that stuff inside constants that last one should also work but be less explicit? |
Great! I didn't know that.
Yeah, I think it is safe to remove it.
If I remember correctly, |
Disable Final for 3.6
@IgnacioJPickering Is there a reason why this is is not merged yet? The |
@zasdfgbnm Sorry I got entretained with something else and forgot about this. The only reason I didn't merge it is I asked for @farhadrgh approval since he modified these files in the public repo, so I can't merge it now unless he reviews |
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, Sorry for the delay!
@farhadrgh @zasdfgbnm This is strange, it is still not allowing me to merge due to deploy-test-pypi, this didn't use to be required for this repo I think, no idea what happened. Do you know how to change this? |
Can you manually uncheck the test in the branch protection plan, or maybe change the settings? https://github.com/roitberg-group/torchani/settings/branches |
@farhadrgh Seems like there is not any way to fine tune the required workflows as they are written right now but it should be possible if I change the name of the job I think |
@farhadrgh works fine now! |
This adds the latest changes to the public upstream made by @farhadrgh and @zasdfgbnm
Unfortunately I found an issue when testing this. It seems that since the files inside resources/ are now not in the .gitignore then git tracks the changes that happen to the files when they are modified after extraction.This happens in both this PR and the current public upstream (aiqm/torchani).
Any idea on how to fix this? it is possible to just touch up the gitignore and checkout the files before doing PR's in the future, but it is uncomfortable. It would be ideal if git could somehow keep a copy of the files frozen but not track further changes.
(to reproduce just clone a new repo, install in a clean conda env and execute energy_force.py for instance)