Radii fix #463

Gabrielgerez · 2023-09-20T14:01:27Z

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 · 2023-09-20T20:43:44Z

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 · 2023-09-20T20:52:22Z

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 · 2023-09-21T06:31:29Z

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 · 2023-09-21T14:10:11Z

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 · 2023-09-21T14:42:41Z

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 · 2023-09-21T15:08:25Z

Codecov Report

All modified lines are covered by tests ✅

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

❗ 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

@@            Coverage Diff            @@
##             master     #463   +/-   ##
=========================================
  Coverage          ?   69.57%           
=========================================
  Files             ?      179           
  Lines             ?    15036           
  Branches          ?        0           
=========================================
  Hits              ?    10461           
  Misses            ?     4575           
  Partials          ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This reverts commit 492ac6c.

Gabrielgerez · 2023-09-25T08:33:11Z

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.

python/mrchem/update_input_parser.py

python/mrchem/validators.py

robertodr

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

less lines of code Co-authored-by: Roberto Di Remigio Eikås <[email protected]>

Rename some elements.

Gabrielgerez · 2023-09-25T12:39:51Z

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

on `update_input_parser.py` and `validators.py`

robertodr · 2023-09-26T06:25:37Z

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

python/mrchem/update_input_parser.py

robertodr · 2023-09-26T06:28:40Z

python/mrchem/validators.py

+            def radiusNotFound(x):
+                # This raises an exception in the list comprehension if the radius is not valid.
+                raise ValueError(
+                    "The vdw-radius of element {x} is not defined in the mantina set".format(x)


f-strings should be preferred:

Suggested change

"The vdw-radius of element {x} is not defined in the mantina set".format(x)

f"The vdw-radius of element {x} is not defined in the mantina set"

robertodr

Two more small comments, but otherwise looks good!

Co-authored-by: Roberto Di Remigio Eikås <[email protected]>

Gabrielgerez · 2023-09-26T07:20:57Z

Two more small comments, but otherwise looks good!

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

…_fix

robertodr

Thanks Gabriel!

Close MRChemSoft#419 We now use qcelemental to fetch Bondi radii, and use alpha=1.2 for hydrogen --------- Co-authored-by: Roberto Di Remigio Eikås <[email protected]>

Gabrielgerez added 2 commits September 20, 2023 14:14

use qcelemental to fetch bondi radii, and use alpha=1.2 for hydrogen

0a75bbb

Update qcelemental version. Was getting error when fetching radii

e4f887f

Gabrielgerez added the bugfix label Sep 20, 2023

Gabrielgerez requested review from stigrj, robertodr and Brakjen September 20, 2023 14:01

Gabrielgerez added 3 commits September 20, 2023 16:44

Add qcelemental to user packages

efc27f7

add qcelemental to mrchem-gha-yml

e311cfe

add qcelemental to mrchem-codecov.yml

9999925

Gabrielgerez added 3 commits September 21, 2023 09:21

Remove qcelemental as a production dependency

6a88a51

move fetch radii from qcel to update input parser

2e3508c

add periodictable to update_input_parser

1d446f8

Add plumbing files to pass tests

492ac6c

Gabrielgerez added 3 commits September 22, 2023 11:21

Revert "Add plumbing files to pass tests"

895404b

This reverts commit 492ac6c.

"second attempt at making tests pass"

e8f2cb5

Update README.md with info about the Elements.

3c5d2a0

robertodr reviewed Sep 25, 2023

View reviewed changes

python/mrchem/update_input_parser.py Outdated Show resolved Hide resolved

robertodr reviewed Sep 25, 2023

View reviewed changes

python/mrchem/update_input_parser.py Outdated Show resolved Hide resolved

robertodr reviewed Sep 25, 2023

View reviewed changes

python/mrchem/update_input_parser.py Outdated Show resolved Hide resolved

robertodr reviewed Sep 25, 2023

View reviewed changes

python/mrchem/validators.py Outdated Show resolved Hide resolved

robertodr requested changes Sep 25, 2023

View reviewed changes

Gabrielgerez and others added 2 commits September 25, 2023 10:56

Update python/mrchem/validators.py

5ffeb78

less lines of code Co-authored-by: Roberto Di Remigio Eikås <[email protected]>

Change radii unit. Handle missing radii better.

7061b70

Rename some elements.

Run isort --profile black

d3a2689

on `update_input_parser.py` and `validators.py`

Gabrielgerez requested a review from robertodr September 25, 2023 12:44

Properly format the files (maybe).

d108d55

robertodr reviewed Sep 26, 2023

View reviewed changes

python/mrchem/update_input_parser.py Show resolved Hide resolved

robertodr reviewed Sep 26, 2023

View reviewed changes

robertodr approved these changes Sep 26, 2023

View reviewed changes

Gabrielgerez and others added 2 commits September 26, 2023 09:18

Properly run formatter and use f-strings

fe4672c

Update python/mrchem/update_input_parser.py

7f015dc

Co-authored-by: Roberto Di Remigio Eikås <[email protected]>

Gabrielgerez requested a review from robertodr September 26, 2023 07:21

Merge branch 'radii_fix' of github.com:Gabrielgerez/mrchem into radii…

d4dfb86

…_fix

robertodr approved these changes Sep 26, 2023

View reviewed changes

robertodr merged commit 5343bf6 into MRChemSoft:master Sep 26, 2023
4 checks passed

robertodr deleted the radii_fix branch September 26, 2023 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Radii fix #463

Radii fix #463

Gabrielgerez commented Sep 20, 2023

stigrj commented Sep 20, 2023

stigrj commented Sep 20, 2023

robertodr commented Sep 21, 2023

Gabrielgerez commented Sep 21, 2023

Gabrielgerez commented Sep 21, 2023

codecov bot commented Sep 21, 2023 •

edited

Loading

Gabrielgerez commented Sep 25, 2023

robertodr left a comment

Gabrielgerez commented Sep 25, 2023

robertodr commented Sep 26, 2023

robertodr Sep 26, 2023

robertodr left a comment

Gabrielgerez commented Sep 26, 2023

robertodr left a comment

	"The vdw-radius of element {x} is not defined in the mantina set".format(x)
	f"The vdw-radius of element {x} is not defined in the mantina set"

Radii fix #463

Radii fix #463

Conversation

Gabrielgerez commented Sep 20, 2023

stigrj commented Sep 20, 2023

stigrj commented Sep 20, 2023

robertodr commented Sep 21, 2023

Gabrielgerez commented Sep 21, 2023

Gabrielgerez commented Sep 21, 2023

codecov bot commented Sep 21, 2023 • edited Loading

Codecov Report

Gabrielgerez commented Sep 25, 2023

robertodr left a comment

Choose a reason for hiding this comment

Gabrielgerez commented Sep 25, 2023

robertodr commented Sep 26, 2023

robertodr Sep 26, 2023

Choose a reason for hiding this comment

robertodr left a comment

Choose a reason for hiding this comment

Gabrielgerez commented Sep 26, 2023

robertodr left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 21, 2023 •

edited

Loading