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

Add a general torch CompositionModel #280

Merged
merged 27 commits into from
Sep 7, 2024
Merged

Add a general torch CompositionModel #280

merged 27 commits into from
Sep 7, 2024

Conversation

PicoCentauri
Copy link
Contributor

@PicoCentauri PicoCentauri commented Jul 2, 2024

Closes #269

Creates an API compatible torch.nn.Module for fitting and predicting compositions weights based on the calculate_composition_weights() function.

Currently, this is the bare implementation and before we include this in the other architectures, I would like to have some feedback.

Contributor (creator of pull-request) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?
  • Use in alchemical_model
  • Use in GAP
  • Use in SOAP-BPNN

📚 Documentation preview 📚: https://metatrain--280.org.readthedocs.build/en/280/

@frostedoyster frostedoyster force-pushed the composition-model branch 5 times, most recently from ceca9d8 to 7a8d131 Compare August 18, 2024 04:38
@Luthaf
Copy link
Member

Luthaf commented Sep 4, 2024

I worked around one issue in the docs (metatensor/metatensor#728), but there seems to be another one now

      File "/home/runner/work/metatrain/metatrain/examples/programmatic/llpr/llpr.py", line 42, in <module>
        model = load_atomistic_model("model.pt", extensions_directory="extensions/")
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/metatrain/metatrain/.tox/docs/lib/python3.12/site-packages/metatensor/torch/atomistic/model.py", line 46, in load_atomistic_model
        return torch.jit.load(path)
               ^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/metatrain/metatrain/.tox/docs/lib/python3.12/site-packages/torch/jit/_serialization.py", line 170, in load
        return wrap_cpp_module(cpp_module)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/runner/work/metatrain/metatrain/.tox/docs/lib/python3.12/site-packages/torch/jit/_recursive.py", line 1027, in wrap_cpp_module
        return torch.jit.RecursiveScriptModule._construct(cpp_module, init_fn)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    AttributeError: type object 'RecursiveScriptModule' has no attribute '_construct'

I wonder if that's related to the release of torch v2.4.1.

@Luthaf
Copy link
Member

Luthaf commented Sep 4, 2024

Hmm, actually, this can be caused by PYTORCH_JIT=O as well. I'll find another wokraround

Copy link
Contributor Author

@PicoCentauri PicoCentauri left a comment

Choose a reason for hiding this comment

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

Very cool. Thanks for trying to finish this.

Once the CI is fixed I can give some more comments.

Comment on lines 217 to 220
if selected_atoms is not None:
composition_energies[output_key] = metatensor.torch.slice(
composition_energies[output_key], "samples", selected_atoms
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can also move this inside the CompositionModel itself?

there is one solution in the LJ example of Guillaume.

I recreated the system in a code.

            # select only real atoms and discard ghosts
            if selected_atoms is not None:
                current_system_mask = selected_atoms.column("system") == system_i
                current_atoms = selected_atoms.column("atom")
                current_atoms = current_atoms[current_system_mask].to(torch.long)

                system_final = System(
                    types=system.types[current_atoms],
                    positions=system.positions[current_atoms],
                    cell=system.cell,
                )

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, I did it in a slightly different way to avoid label conflicts but it's now there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes nice implementation.

from ....utils.data.dataset import Dataset, get_atomic_types


def calculate_composition_weights(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is not needed anymore?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately this needs to be kept for the alchemical model (notice that it changed directories), which works in a way that doesn't allow me to change things without changing the alchemical model code itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay!

Comment on lines +276 to +277
if not self.training:
# at evaluation, we also add the composition contributions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NICE!

@frostedoyster frostedoyster merged commit e6d3927 into main Sep 7, 2024
12 checks passed
@frostedoyster frostedoyster deleted the composition-model branch September 7, 2024 10:12
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.

Create a torch.nn.Module for the composition weights
3 participants