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

Adding function to calculate HF orbitals and made minor reformat - Ferminet #3466

Merged
merged 13 commits into from
Jul 26, 2023

Conversation

shaipranesh2
Copy link
Contributor

@shaipranesh2 shaipranesh2 commented Jul 7, 2023

Description

Added prepare_hf_solution function to calculate hartree fock solution required for pertaining.
Reformatted the code into Ferminet class (class where ferminet model will implemented) and FerminetModel class (driver class providing all the necessary data to Ferminet class)

Fix #(issue)
Fixed the calculation of the spin of the molecule system

Type of change

Please check the option that is related to your PR.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
    • In this case, we recommend to discuss your modification on GitHub issues before creating the PR
  • Documentations (modification for documents)

Checklist

  • My code follows the style guidelines of this project
    • Run yapf -i <modified file> and check no errors (yapf version must be 0.32.0)
    • Run mypy -p deepchem and check no errors
    • Run flake8 <modified file> --count and check no errors
    • Run python -m doctest <modified file> and check no errors
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

except ModuleNotFoundError:
pass


@pytest.mark.torch
def test_prepare_input_stream():
Copy link
Member

Choose a reason for hiding this comment

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

Why are we deleting this older test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old test checks for input streams shape, which is nothing but each electrons distance from each nuclei, this can be done using deepchem utils pairwise distance itself and doesn't require tests!

@@ -22,9 +24,55 @@ def test_f(x: np.ndarray) -> np.ndarray:
return 2 * np.log(np.random.uniform(low=0, high=1.0, size=np.shape(x)[0]))


class Ferminet:
class Ferminet(torch.nn.Module):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this new layer to torch_layers.csv and layers.rst in the docs/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I do this once the model is fully completed? As I am breaking the chunk of PR into smaller ones and am adding one function by function.

@@ -22,9 +24,55 @@ def test_f(x: np.ndarray) -> np.ndarray:
return 2 * np.log(np.random.uniform(low=0, high=1.0, size=np.shape(x)[0]))


class Ferminet:
class Ferminet(torch.nn.Module):
"""Approximates the log probability of the wave function of a molecule system using DNNs.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a usage example here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I do this once the model is fully completed? As I am breaking the chunk of PR into smaller ones and am adding one function by function.

self.determinant = determinant


class FerminetModel(TorchModel):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this class to the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I do this once the model is fully completed? As I am breaking the chunk of PR into smaller ones and am adding one function by function.

self.nucleon_coordinates = nucleon_coordinates
self.seed = seed
self.batch_no = batch_no
self.spin = spin
self.ion_charge = charge

def prepare_input_stream(self,) -> Tuple[Any, Any, Any, Any]:
Copy link
Member

Choose a reason for hiding this comment

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

Why are we deleting this helper function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This helper function computes pairwise distances of each electron from nuclei and can be done in forward itself

@@ -124,46 +164,77 @@ def prepare_input_stream(self,) -> Tuple[Any, Any, Any, Any]:
self.electron_no[int(electro_neg[-1 - iter][0])][0] += 1

total_electrons = np.sum(self.electron_no)
self.up_spin = (total_electrons + 2 * self.spin) // 2
self.down_spin = (total_electrons - 2 * self.spin) // 2

Copy link
Member

Choose a reason for hiding this comment

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

Could you give me a summary of the refactoring changes you've made here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes I have done are:

  1. added a new helper function prepare_hf which gives hartree fock orbitals values for pretraining.
  2. Deleted the code to prepare input streams as it should be done in forwards, as we need the gradient of output wrt to the electron's coordinates we are going to pass

For now, this model is empty and have added the helper function only. In successive PRs I will complete the forward function and add example

Copy link
Member

@rbharath rbharath left a comment

Choose a reason for hiding this comment

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

Good start. I have some questions and requests for more docs below

@shaipranesh2
Copy link
Contributor Author

shaipranesh2 commented Jul 26, 2023

@rbharath I have reformatted to numpydocs style.
As this model is not complete now, I thought of not adding it to the layers.rst and in other docs.
I have added a helper function for pertaining and removed unnecessary functions for calculating input streams.
In the next PR I will be adding forward function of the model!
This PR is mainly about DQC test

Is this fine?

Copy link
Member

@rbharath rbharath left a comment

Choose a reason for hiding this comment

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

LGTM

@rbharath rbharath merged commit 72c701a into deepchem:master Jul 26, 2023
24 of 33 checks passed
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.

None yet

2 participants