AtomicData reads charge and spin from ASE calcualator for omol task#1794
AtomicData reads charge and spin from ASE calcualator for omol task#1794alex-l-m wants to merge 3 commits intofacebookresearch:mainfrom
Conversation
|
Hi @alex-l-m! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
| [ | ||
| atoms.info.get("charge", 0) | ||
| if r_data_keys is not None and "charge" in r_data_keys | ||
| if (r_data_keys is not None and "charge" in r_data_keys) or (r_data_keys is None and task_name == "omol") |
There was a problem hiding this comment.
Thanks for getting this started!
Assuming tests pass, i think we could get rid of this conditional entirely. atoms.info.get("charge/spin", 0) should be relevant for all our tasks. r_data_keys shouldn't matter here for our other tasks. @rayg1234 thoughts?
There was a problem hiding this comment.
ya we already default to the "right" values for omol vs not omol tasks here, we probably dont need r_data_keys dont need specific omol logic here
3 participants
One possible way to address issue #1793
AtomicData reads
"charge"and"spin"fromatoms.infofor task"omol", ifr_data_keysis not set.If
r_data_keysis set, it overrides this behavior, so it would need to include"charge"and"spin"as keys.Also changes FAIRChemCalculator to not use
r_data_keys.The big thing I'm not sure of is making reading from
atoms.infoconditional on task"omol".That seems to match the FAIRChemCalculator docstring, although I see that reading charge and spin is not currently conditional on this task.
This change would break anything that requires
"charge"and"spin"to be read fromatoms.infofor any other tasks.Even if that's not a breaking change now, maybe it's not what you want for the future?
Also changes the example in the docs that motivated me here.