MRChemSoft / mrchem

MultiResolution Chemistry
GNU Lesser General Public License v3.0
27 stars 21 forks source link

Radii fix #463

Closed Gabrielgerez closed 11 months ago

Gabrielgerez commented 11 months ago

Finally I implement a simple fix for the default radii for the solvent. This has been an open issue since #419. Its not the prettiest fix, but I think it does its job.

stigrj commented 11 months ago

I would really like to avoid introducing qcelemental as a runtime dependency.

If I understand the PhysicalConstants analogue correctly, qcelemental is used only as a dev dependency when updating the input template.yml file. Then, in update_input_parser.py, the parameters are fetched from qcelemental and put as default values in a dedicated Constants section in the regular input. Not sure if we want to pass all these radii through the standard input file, though?

How about having update_input_parser.py write directly to the periodictable.py file?

Any thoughts @robertodr ?

stigrj commented 11 months ago

Actually, perhaps it's not such a bad idea to pass the entire periodic table through the regular input file? In general we would of course fall back to default values, so the regular user would never have to bother with it. But then the advanced user could easily change a single parameter directly in the input file for use in a particular run, without hard coding a change in a static periodictable.py file.

robertodr commented 11 months ago

I agree with Stig that we should not add Python packages as required production dependencies. They should all stay as development dependencies and the data needed from them should be saved in the mrchem Python module, which only needs a working Python interpreter to do its job.

I think it's OK to update periodictable.py using qcelemental and I think passing it through the parser as done with the constants is a good idea, since it's exposed to the advanced user.

Gabrielgerez commented 11 months ago

I have now added the periodic table to the template through the update_input_parser. There are some issues, for example there are elements for which there are no valid vdw-radii in the mantina set. Another one is that the element No is confusing the parser since no is a reserved yaml keyword for a boolean false (also the yes keyword for true)

Gabrielgerez commented 11 months ago

Tests are failing, have added a lot of the files that are under python/mrchem/input_parser/plumbing/pyparsing as it seems there is a bit of version issues. Will most likely revert these changes later

codecov[bot] commented 11 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (master@d8f2b39). Click here to learn what that means.

:exclamation: Current head 7f015dc differs from pull request most recent head d4dfb86. Consider uploading reports for the commit d4dfb86 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #463 +/- ## ========================================= Coverage ? 69.57% ========================================= Files ? 179 Lines ? 15036 Branches ? 0 ========================================= Hits ? 10461 Misses ? 4575 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Gabrielgerez commented 11 months ago

Everything should be working now. Have some issues with Nobellium and elements without radius entry in the mantina set, but otherwise it should be ok.

Gabrielgerez commented 11 months ago

I think you need to run Black and isort on periodictable.py and validators.py

Do we have a special configuration file for this? I ran isort --profile black on both files, but it barely changed anything

robertodr commented 11 months ago

So for formatting it's: black <files-to-format> and for sorting the imports it's: isort <files-to-sort>

Gabrielgerez commented 11 months ago

Two more small comments, but otherwise looks good!

Alright, i have now added all suggestions :) We can finally fix this bug