Refactoring by p #72

prtos · 2024-03-29T18:52:14Z

Fixes #65.

Checklist:

Was this PR discussed in a issue? It is recommended to first discuss a new feature into a GitHub issue before opening a PR.
Add tests to cover the fixed bug(s) or the new introduced feature(s) (if appropriate).
Update the API documentation is a new function is added or an existing one is deleted.

FNTwin

There is a missing file.
I really like the Enum adaptation of the level of theory.
Currently the branch doesn't pass most of the actions so it need some fixes.

FNTwin · 2024-04-01T17:47:33Z

openqdc/datasets/interaction/base.py

@@ -61,7 +48,7 @@ def data_types(self):
        }

    def __getitem__(self, idx: int):
-        shift = IsolatedAtomEnergyFactory.max_charge
+        shift = MAX_CHARGE


For the interaction dataset we don't really need the Isolated Atom Energies so we can rework the getitem method in case

the code just give zeros isolated atom energies for all the interaction methods

FNTwin · 2024-04-01T17:55:12Z

openqdc/methods/__init__.py

I think all the Enums should be in a standalone python file enums.py

FNTwin · 2024-04-01T17:56:33Z

openqdc/methods/__init__.py

+
+
+class PotentialMethod(QmMethod): #SPLIT FOR INTERACTIO ENERGIES AND FIX MD17
+    B1LYP_VWN5_DZP                      = Functional.B1LYP_VWN5, BasisSet.DZP, 0


While we can do more analysis on the cost of the methods, we can just have already a scale of cost with 0 being a SE, 1 DFT, 2 CCSD etc

FNTwin · 2024-04-01T18:00:04Z

openqdc/methods/__init__.py

+    B3LYP_VWN5      = "b3lyp(vwn5)"
+    B3LYP_S_VWN5    = "b3lyp*(vwn5)"
+    B3LYPD          = "b3lyp-d"
+    B3LYP_D3_BJ      = "b3lyp-d3(bj)"


As we are adding more structure I think we should separate the corrections from the functionals?

Example:
B3LYP_D3_BJ is a B3LYP functional with a dispersion correction (d3) + Becke Johnson damping (bj) correction

FNTwin · 2024-04-01T18:01:28Z

openqdc/methods/atom_energies.py

+with open(os.path.join(os.path.dirname(__file__), "atom_energies.txt")) as fd:
+    atom_energy_collection = ast.literal_eval(fd.read())
+    atom_energy_collection = {k.lower():v for k, v in atom_energy_collection.items()}


"atom_energies.txt" file is currently missing.
Also, maybe json is easier to extend with new values?

FNTwin · 2024-04-01T18:02:24Z

tests/test_dummy.py

+# def test_is_at_factory():
+#     res = IsolatedAtomEnergyFactory.get("mp2/cc-pvdz")
+#     assert len(res) == len(ISOLATED_ATOM_ENERGIES["mp2"]["cc-pvdz"])
+#     res = IsolatedAtomEnergyFactory.get("PM6")
+#     assert len(res) == len(ISOLATED_ATOM_ENERGIES["pm6"])
+#     assert isinstance(res[("H", 0)], float)


The tests shouldn't be disabled but adapted

FNTwin · 2024-04-01T18:04:50Z

openqdc/methods/__init__.py

+from openqdc.methods.atom_energies import atom_energy_collection, to_e_matrix
+
+
+class QmType(StrEnum):


Where are we planning to use this Enum?

FNTwin · 2024-04-01T18:05:04Z

openqdc/methods/__init__.py

+
+
+class QmType(StrEnum):
+    FF  = "Empirical Force Field"


Just Force Field is enough

shenoynikhil · 2024-04-01T23:21:35Z

Just ran the pre-commit in my previous commit. @prtos If you push the atomic_energies.txt, the tests should run ideally.

shenoynikhil · 2024-04-01T23:26:40Z

openqdc/datasets/interaction/splinter.py

+        # "sapt0/jun-cc-pV(D+d)Z_unscaled", #TODO: we need to pick the unscaled version only here
+        # "sapt0/jun-cc-pV(D+d)Z_es_unscaled",
+        # "sapt0/jun-cc-pV(D+d)Z_ex_unscaled",
+        # "sapt0/jun-cc-pV(D+d)Z_ind_unscaled",
+        # "sapt0/jun-cc-pV(D+d)Z_disp_unscaled",
+        # "sapt0/jun-cc-pV(D+d)Z_scaled",
+        # "sapt0/jun-cc-pV(D+d)Z_es_scaled",
+        # "sapt0/jun-cc-pV(D+d)Z_ex_scaled",
+        # "sapt0/jun-cc-pV(D+d)Z_ind_scaled",
+        # "sapt0/jun-cc-pV(D+d)Z_disp_scaled",
+        # "sapt0/aug-cc-pV(D+d)Z_unscaled",
+        # "sapt0/aug-cc-pV(D+d)Z_es_unscaled",
+        # "sapt0/aug-cc-pV(D+d)Z_ex_unscaled",
+        # "sapt0/aug-cc-pV(D+d)Z_ind_unscaled",
+        # "sapt0/aug-cc-pV(D+d)Z_disp_unscaled",
+        # "sapt0/aug-cc-pV(D+d)Z_scaled",
+        # "sapt0/aug-cc-pV(D+d)Z_es_scaled",
+        # "sapt0/aug-cc-pV(D+d)Z_ex_scaled",
+        # "sapt0/aug-cc-pV(D+d)Z_ind_scaled",
+        # "sapt0/aug-cc-pV(D+d)Z_disp_scaled",


Can we get rid of these comments if there energy values aren't being used.

shenoynikhil · 2024-04-01T23:33:57Z

openqdc/methods/__init__.py

+from openqdc.methods.atom_energies import atom_energy_collection, to_e_matrix
+
+
+class QmType(StrEnum):


Is QmType the right word for this Enum? Given that there are Force Fields, Semi-Empirical keys under this.

shenoynikhil · 2024-04-01T23:35:55Z

openqdc/methods/__init__.py

+if __name__ ==  "__main__":
+    for method in PotentialMethod:
+        (str(method), len(method.atom_energies_dict))


I would suggest not having this __main__ code here, maybe you added this for testing. We can have a separate test for iterating through potential method?

shenoynikhil

Minor comments, should be go to go if (necessary) tests are added and all tests pass. Just make sure to run pre-commit before you commit.

Just a 1 time cost,

pre-commit install (to setup)
pre-commit run --all-files

The next time you add and try to commit, this will automatically run.

…ry/openQDC into refactoring_by_P

py3.8 compatibility, manual fixes to atom energies, pkgutils + fixes on gh action tests

prtos added 6 commits March 28, 2024 17:25

create method enums and refactor the rest accordingly

dc74dd6

adding new files

c34e53f

fix import issues and update API doc files

ef4581d

ATOM_TABLE import fix

05d39c9

QmInteractionMethod added

5ee18c2

rename QmMethod classes

69c081f

prtos changed the base branch from main to develop March 29, 2024 18:52

prtos requested review from FNTwin and shenoynikhil and removed request for FNTwin and shenoynikhil March 29, 2024 18:52

prtos assigned shenoynikhil Mar 29, 2024

FNTwin reviewed Apr 1, 2024

View reviewed changes

prtos and others added 2 commits April 1, 2024 18:22

adding a missing file

b97cf25

pre-commit

4eff5f4

shenoynikhil reviewed Apr 1, 2024

View reviewed changes

shenoynikhil approved these changes Apr 1, 2024

View reviewed changes

FNTwin changed the base branch from develop to release April 2, 2024 16:48

prtos and others added 8 commits April 2, 2024 18:28

added correction method

e3c4fcf

Merge branch 'refactoring_by_P' of https://github.com/OpenDrugDiscove…

a0ad3ab

…ry/openQDC into refactoring_by_P

added correction method

fe2d9bd

fixed pre-commits

496267f

missing import

b4bb0f3

str fix in cli

085b933

updated python version since strenum from py>3.11

5fe28ce

py3.8 compatibility, manual fixes to atom energies

05a6dbf

FNTwin and others added 3 commits April 3, 2024 13:16

pkgutils

65a8a12

some debugging

3b4199f

Merge pull request #75 from OpenDrugDiscovery/corrections

2513695

py3.8 compatibility, manual fixes to atom energies, pkgutils + fixes on gh action tests

This was linked to issues Apr 3, 2024

Standardize Level of Theory #65

Closed

Optimize Atomization Energies #64

Closed

Solved merged issues, added NONE PotentialMethod

2f6eddb

FNTwin merged commit 9574974 into release Apr 3, 2024
5 checks passed

FNTwin deleted the refactoring_by_P branch April 23, 2024 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring by p #72

Refactoring by p #72

prtos commented Mar 29, 2024 •

edited by shenoynikhil

Loading

FNTwin left a comment

FNTwin Apr 1, 2024

prtos Apr 2, 2024

FNTwin Apr 1, 2024

FNTwin Apr 1, 2024

FNTwin Apr 1, 2024

FNTwin Apr 1, 2024

FNTwin Apr 1, 2024

FNTwin Apr 1, 2024

FNTwin Apr 1, 2024

shenoynikhil commented Apr 1, 2024

shenoynikhil Apr 1, 2024

shenoynikhil Apr 1, 2024

shenoynikhil Apr 1, 2024 •

edited

Loading

shenoynikhil left a comment



		class PotentialMethod(QmMethod): #SPLIT FOR INTERACTIO ENERGIES AND FIX MD17
		B1LYP_VWN5_DZP = Functional.B1LYP_VWN5, BasisSet.DZP, 0

		from openqdc.methods.atom_energies import atom_energy_collection, to_e_matrix


		class QmType(StrEnum):

Refactoring by p #72

Refactoring by p #72

Conversation

prtos commented Mar 29, 2024 • edited by shenoynikhil Loading

FNTwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenoynikhil commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenoynikhil Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

shenoynikhil left a comment

Choose a reason for hiding this comment

prtos commented Mar 29, 2024 •

edited by shenoynikhil

Loading

shenoynikhil Apr 1, 2024 •

edited

Loading