Added conformers interface to plams #319
Conversation
switched to uv some commands such that you do not need to pollute the amspython with tools like black and nbconvert
as mentioned in #319 (comment)
doc/source/examples/COSMORSConformers/cosmors_conformers.ipynb.rst
Outdated
Show resolved
Hide resolved
| @@ -1,55 +0,0 @@ | |||
| #!/usr/bin/env amspython | |||
| from scm.plams import * | |||
| from scm.conformers import ConformersJob, ConformersResults | |||
There was a problem hiding this comment.
This is used as an example in the nightly tests - so either that needs to be updated or this can be added back in
| return el | ||
|
|
||
| @requires_optional_package("matplotlib") | ||
| def plot_conformers( |
There was a problem hiding this comment.
Should this be here or in tools/plot for consistency?
| * Make commits of logical units | ||
| * Make sure your commit messages are informative | ||
| * Make sure you have added the necessary tests for your changes | ||
| * Add examples: take a look at the corresponding [README.md](./examples/README.md) |
There was a problem hiding this comment.
Also update CHANGELOG (here and for your change!)
There was a problem hiding this comment.
should I mention that I changed the CONTRIBUTING.md file in the CHANGELOG?
dormrod
force-pushed
the
GiulioBenedini/Conformers
branch
3 times, most recently
from
cf29976 to
d572a57
Compare
| forcefield_params_from_kf, | ||
| ) | ||
| from scm.plams.interfaces.adfsuite.inputparser import get_system_blocks_as_molecules_from_input, input_to_settings | ||
| from scm.plams.interfaces.adfsuite.inputparser import ( |
There was a problem hiding this comment.
For future PRs, please can you turn off the auto-reordering of imports? It bloats the diff and makes it a bit harder to review
| __all__ = ["ConformersJob", "ConformersResults"] | ||
|
|
||
|
|
||
| class ConformersCollection: |
There was a problem hiding this comment.
I see that this is actually not just a straight "lift and shift".
You have replaced the old Conformers usage to remove the dependency on scm.conformers with this class. This seems like it might have consequences - will some functionality be lost? The plan is I assume to remove the old implementation in scm.conformers?
Might be worth asking @rebadam if you have not already discussed with her.
There was a problem hiding this comment.
I am quite ok with the PLAMS wrapper around the conformers tool being very basic. Though I would prefer if we could point the user to the conformers library we already have if they want more functionality. But I don't like so much that you wrote a whole new Conformers class. It is very basic, which is good, but I am afraid people will start adding to it, which will result in a lot of double coding. I would prefer if the functionality of the ConformersContainer class would just be folded into the Results class directly. The conformers property of the results class could just be deprecated.
| if not path.name.endswith("conformers.xyz"): | ||
| raise ValueError(f"{path=} not valid") | ||
|
|
||
| assert path.exists() |
There was a problem hiding this comment.
(don't use assert in production code)
| raise ValueError(f"{path=} not valid") | ||
|
|
||
| assert path.exists() | ||
| molecules = read_all_molecules_in_xyz_file(str(path)) |
There was a problem hiding this comment.
Using the xyz files for example loses any bonding information?
There was a problem hiding this comment.
Yes, I agree here. It would be super simple to just read from the conformers.rkf file, which also has higher precision for the coordinates. The RKF file just has the geometries and energies in the History section.
|
If I run ConformersJob from a different Python environment, which Python environment does the workflow use—the default amspython or the one I launched it from? |
It will be called with: so amspython |
The plams interface instead of living in
scm.conformershas moved toscm.plams. This is in accordance with what @mhellstr and I discussed, we think is a cleaner approach and more inline with plams design.Added method
plot_conformerstoConformersResults.Updated the notebooks examples but not the .py files yet.
Let me know what do you think.