Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Isolated at en #18

Merged
merged 22 commits into from
Nov 2, 2023
Merged

Isolated at en #18

merged 22 commits into from
Nov 2, 2023

Conversation

FNTwin
Copy link
Collaborator

@FNTwin FNTwin commented Oct 27, 2023

Various stuffs:

Other things:

  • Implement the correct unit for every dataset
  • Implement the correct level of theory
  • Implement the correct force sign
  • Implement the mkdocs serve with a simple doc
  • Clean the env.yml and remove unwanted deps
  • Implement a utility method to save extend xyz files
  • Implement all the isolated atom energies and wrapped them into a nice and clean dict
  • Implement factory method to automatically get the correct energy from the class
  • Changes the init of the base class for the cache selection
  • Better inheritance in the child classes
  • Implementation of lazy loading of the modules
  • No more NablaDFT HamiltonianDatabase initialization everytime you call the dataset module

To do :

  • solve Unique Atom Types and Charges #5 in a bit
  • correct way to apply isolated_atom_energies (right now we are not considerating the charge and the format for the dict is Tuple(str , int) as (Name of the element , charge) )
  • solve the merge issue

@FNTwin FNTwin requested a review from prtos as a code owner October 27, 2023 19:29
src/openqdc/datasets/base.py Outdated Show resolved Hide resolved
src/openqdc/datasets/ani.py Outdated Show resolved Hide resolved
src/openqdc/datasets/base.py Show resolved Hide resolved
def __init__(self, energy_unit=None, distance_unit=None) -> None:
super().__init__(energy_unit=energy_unit, distance_unit=distance_unit)
# Energy in hartree, all zeros by default
atomic_energies = np.zeros((MAX_ATOMIC_NUMBER,), dtype=np.float32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we set all atomic energies to zeros here? don't we have them calculated?

Copy link
Collaborator Author

@FNTwin FNTwin Oct 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current atomic energies workflow implemented in main is wrong as we are not getting the isolated atom energies considering the formal charge of the atom.
Right now the energies are indexed by the chemical number but this is not taking account that elements with different charges have different energies, so we should be indexing by a Tuple(Chemical species, Formal Charge)

This is a workaround to continue using the package while I take care about it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented a way to get the correct e0 considering the formal charges. See 928716b

src/openqdc/datasets/orbnet_denali.py Outdated Show resolved Hide resolved
src/openqdc/datasets/qm7x.py Outdated Show resolved Hide resolved
src/openqdc/datasets/qmugs.py Outdated Show resolved Hide resolved
src/openqdc/datasets/tmqm.py Outdated Show resolved Hide resolved
src/openqdc/datasets/waterclusters3_30.py Show resolved Hide resolved
@FNTwin
Copy link
Collaborator Author

FNTwin commented Oct 31, 2023

Implemented:

  • Docstrings
  • Docs + tutorial + some API on mkdocs
  • SOAP vectors calculation for dataset chemical space

@FNTwin FNTwin merged commit 33af696 into main Nov 2, 2023
3 of 5 checks passed
@FNTwin FNTwin deleted the isolated_at_en branch November 2, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants