[WIP] Integration and test of RSS code #84

YuanbinLiu · 2024-07-29T10:43:42Z

This PR primarily integrates the RSS code with the phonon code.

Todo list

Refactor the gap_fitting part in MLIPFitMaker, as it is kind of specific to GAP and phonon tasks.
Separate the DataPreprocessing job and the machine_learning_fit job because DataPreprocessing might need to be used with active learning later, and we need to ensure its flexibility.
Complete the interface to XPOT so that one can perform optimization and conduct pacemaker fitting.

This reverts commit 0e1bdbc, reversing changes made to 7b1012b.

This reverts commit 12d88b5.

This reverts commit 7b1012b.

jla-gardner

Nice work here 😄

I've left a few comments regarding style - most of my points are applicable across the PR but I've made them in just one representative case.

The nequip_fitting and the default parameters you have selected seem reasonable to me!

jla-gardner · 2024-07-30T09:33:11Z

autoplex/data/common/jobs.py

+    random_seed: int = None,
+):
+    """
+    Job to sample training configurations from trajs of MD/RSS.


Would you consider changing instances of "trajs" to "trajectories"? To me, the former is a needless contraction, particularly in a docstring

Yes, sounds good! It has been changed now.

jla-gardner · 2024-07-30T09:34:10Z

autoplex/data/common/jobs.py

+
+    Parameters
+    ----------
+    selection_method : str, optional


if you are expecting one of several options, why not type hint this with Literal["cur", "bcur", ...]?

Using type hint is indeed better. I have implemented that change.

jla-gardner · 2024-07-30T09:34:47Z

autoplex/data/common/jobs.py

+        - 'cur': Pure CUR selection.
+        - 'bcur': Boltzmann flat histogram in enthalpy, then CUR.
+        - 'random': Random selection.
+        - 'uniform': Uniform selection. Default is None. If None, then default to random.


Why default to None here, when you could just default to "random"?

Sure, let‘s make it even simpler.

jla-gardner · 2024-07-30T09:37:29Z

autoplex/data/common/jobs.py

+        "energy_label": "energy",
+    }
+
+    if bcur_params is not None:


What happens if the user specifies bcur_param={"soap_params": {"atom_sigma": 0.5}} - do you want to keep the rest of the default soap params? Or require the user to provide all of these.

Then it will only change the value of "atom_sigma". The rest of the default soap params will be kept. The user does not need to provide all parameters.

jla-gardner · 2024-07-30T09:40:13Z

autoplex/data/common/jobs.py

+        List of structures for sampling. Default is None.
+
+    traj_info : list, optional
+        List of dictionaries containing trajectory information. Each dictionary should


Type hint should be list[dict] here as a minimum I think. If you want to give more info, you could consider creating e.g. a TypedDict to indicate to the the typing tools that you are expecting these keys.

Agree. This is done by traj_info: list[dict[str, Union[str, float]]] = None.

autoplex/data/common/jobs.py

jla-gardner · 2024-07-30T09:49:37Z

autoplex/data/common/utils.py

+    if recursive:
+        for element in atoms_object:
+            if isinstance(element, Iterable) and not isinstance(
+                element, (str, bytes, ase.atoms.Atoms, ase.Atoms)


The logic here is not mirrored in the docstring

I just made some changes.

autoplex/fitting/common/jobs.py

jla-gardner · 2024-07-30T09:59:23Z

autoplex/fitting/common/utils.py

@@ -634,6 +650,9 @@ def m3gnet_fitting(
    max_n: int = 4,
    device: str = "cuda",
    test_equal_to_val: bool = True,
+    ref_energy_name: str = "REF_energy",


Thi nequip_fitting functions appears to do what it says on the tin, but is quite hacky. Rather than writing a raw text file to a file, have you considered yaml dumping a dictionary of the config instead?

I think we still have a few rather hacky parts in the fitting part of autoplex. Any further suggestions or pull requests would be appreciated.

In the short term, I think this is fine to be merged.

In the medium term, we could consider using the functions exposed in the nequip.scripts.train/deploy submodules to just run things from within python.

In the long term, a new fitting framework is being made that unifies a lot of these model architectures - integrating this will simplify things here significantly.

Very much agreed.

We should try to get a minimum viable product out first and then subsequently improve it. (I also very much hope this whole pytorch version and conflict mess between different fitting frameworks won't keep us too busy in the future. We might have to think about other installation strategies then.... )

Good point and agree with all views. For nequip, we definitely can switch to yaml. But for J-ACE, I don't find a good solution.

@jla-gardner, do you think the hyperparameters listed for nequip are the most important ones? Besides these, do you think there's any other hyperparameter that might have been overlooked?

These hyperparameters are definitely the most important. Looks good to me!

jla-gardner · 2024-07-31T06:59:06Z

autoplex/data/common/jobs.py

+
+import logging 
+
+logging.basicConfig(level=logging.DEBUG, format='[%(levelname)s] %(message)s')


For now, this will do. In a subsequent PR we should extract this into its own file, and adopt its use across the whole repo.

QuantumChemist · 2024-08-02T04:41:12Z

@YuanbinLiu @QuantumChemist my suggestion for now would be: @QuantumChemist takes a closer look tomorrow and then both of you meet to clarify the last questions. For some reason, one can sometimes not see very well when whole code blocks have been moved to another file.

Most things have been cleared up. I will check the code to see if it still runs as intended. Still, to give my part of the approval to this PR, the following (EDIT!) three and an optional fourth criteria have to be met:

The unit tests have to be restored and adjusted to the code changes, especially if the new unit tests are not rss specific (then the rss unit tests are redundant)
a proper RSS static maker for VASP has to be written
type hints for every function.that isn't a Flow or job object (EDIT!)
separate files for the MLIP default settings (this is optional if it's not easily done)

JaGeo · 2024-08-02T05:38:16Z

@YuanbinLiu I agree with @QuantumChemist here on the required changes.

Additionally, the documentation generation needs to be fixed. I don't know why the CI is failing. Unfortunately, this needs to be investigated as well.
In a subsequent PR, all changes in this PR should be reflected in the documentation and tutorials of the code
In another or the same subsequent PR, additional documentation for the RSS part should be added.

JaGeo · 2024-08-02T05:45:12Z

One slight addition for the unit tests: I suggest having a common test file for all fitting-related unit tests. I think adding all tests to the RSS-related fitting tests is not a perfect solution, as I would not have expected all other tests for the fitting code to be in there as well.

JaGeo

I just marked one of my requests now in the pull request itself.

JaGeo · 2024-08-02T05:50:55Z

tests/rss/test_ml_fitting.py

+from __future__ import annotations
+from autoplex.fitting.common.flows import MLIPFitMaker
+import shutil
+from pathlib import Path
+from jobflow import run_locally
+
+
+def test_gap_fit_maker(test_dir, memory_jobstore):
+
+    database_dir = test_dir / "fitting/rss_training_dataset/"
+
+    gapfit = MLIPFitMaker().make(
+        auto_delta=False,
+        glue_xml=False,
+        twob={"delta": 2.0, "cutoff": 4},
+        threeb={"n_sparse": 10},
+        preprocessing_data=False,
+        database_dir=database_dir    
+        )
+
+    responses = run_locally(
+        gapfit, ensure_success=True, create_folders=True, store=memory_jobstore
+    )
+
+    assert Path(gapfit.output["mlip_path"].resolve(memory_jobstore)).exists()


I would suggest moving these tests to the general fitting part.

Good suggestion! It is done.

QuantumChemist · 2024-08-02T06:34:41Z

Another thing that is also missing that I notice now are type hints for every function.that isn't a Flow or job object.

JaGeo · 2024-08-02T06:41:08Z

Another thing that is also missing that I notice now are type hints for every function.that isn't a Flow or job object.

This could be the problem for the documentation generation, btw

QuantumChemist · 2024-08-02T06:49:41Z

autoplex/fitting/common/utils.py

@@ -213,7 +217,7 @@ def gap_fitting(
            title="Data error metrics",
            energy_limit=0.005,
            force_limit=0.1,
-            species_list=species_list,
+            species_list=species_list,  # species list is required here


species_list is required here to post_process GAP data

I suggest that maybe a new argument like pairplot: bool = None is better. If pairplot is used, we can use the existing function to get species_list instead of passing it (the process is quick). Because species_list does not seem related to all model fitting.

Could we add this to our list of potential todos within an issue? Maybe to reach consensus here fast, we can keep the species_list for now and improve this later? @YuanbinLiu would this work for you?

yeah, sounds sensible! Let's keep species_list at the moment.

You could add an Issue with your suggestion and then we can discuss this there! It can also be a more general issue if you have additional points!

Species_list will get related to all models, once such analysis plots are implemented for them as well.

Let's just discuss this in a separate issue how to handle this. For now, let's be pragmatic.

QuantumChemist

There are some docstrings missing

QuantumChemist · 2024-08-02T07:18:08Z

autoplex/fitting/common/utils.py

+    ref_energy_name: str = "REF_energy",
+    ref_force_name: str = "REF_forces",
+    ref_virial_name: str = "REF_virial",


docstrings are missing

thanks, they are added now.

QuantumChemist · 2024-08-02T07:18:29Z

autoplex/fitting/common/utils.py

+    ref_energy_name: str = "REF_energy",
+    ref_force_name: str = "REF_forces",
+    ref_virial_name: str = "REF_virial",


docstrings are missing

QuantumChemist · 2024-08-02T07:19:07Z

autoplex/fitting/common/utils.py

+    ref_energy_name: str = "REF_energy",
+    ref_force_name: str = "REF_forces",
+    ref_virial_name: str = "REF_virial",


docstrings missing

QuantumChemist · 2024-08-02T07:19:23Z

autoplex/fitting/common/utils.py

+    ref_energy_name: str = "REF_energy",
+    ref_force_name: str = "REF_forces",
+    ref_virial_name: str = "REF_virial",


docstrings missing

QuantumChemist · 2024-08-02T07:38:23Z

@YuanbinLiu I agree with @QuantumChemist here on the required changes.

* [ ]  Additionally, the documentation generation needs to be fixed. I don't know why the CI is failing. Unfortunately, this needs to be investigated as well.

* [ ]  In a subsequent PR, all changes in this PR should be reflected in the documentation and tutorials of the code

* [ ]  In another or the same subsequent PR, additional documentation for the RSS part should be added.

@YuanbinLiu @QuantumChemist my suggestion for now would be: @QuantumChemist takes a closer look tomorrow and then both of you meet to clarify the last questions. For some reason, one can sometimes not see very well when whole code blocks have been moved to another file.

Most things have been cleared up. I will check the code to see if it still runs as intended. Still, to give my part of the approval to this PR, the following (EDIT!) three and an optional fourth criteria have to be met:
* [ ]  The unit tests have to be restored and adjusted to the code changes, especially if the new unit tests are not rss specific (then the rss unit tests are redundant)

* [ ]  a proper RSS static maker for VASP has to be written

* [ ]  type hints for every function.that isn't a Flow or job object (EDIT!)

* [ ]  separate files for the  MLIP default settings (this is optional if it's not easily done)

I have now carefully checked and adjusted the code where needed. The only issues that are left now are the ones addressed here. Of course, as always, I'm happy to help with some of the changes, like e.g. the unit tests, if it's not possible for you to restore them by one click in the IDE etc. 😄

QuantumChemist · 2024-08-03T06:23:25Z

I'd like to kindly remind everyone about our contribution guidelines as well regarding variable names, type-hints etc.:
https://github.com/JaGeo/autoplex/blob/main/docs/dev/contributing.md

Guidelines for contributions

Please write unit tests; this is a requirement for any added code to be accepted. (Automated testing will be performed using pytest; you can look into the tests folder for examples).
Please ensure high coverage of the code based on the tests (you can test this with coverage).
Please use numpy docstrings (use an IDE and switch on this docstring type; you can check examples in our code base; the docstring should be useful for other people) <---
Please ensure that type hints are added for each variable, function, class, and method (this helps code readability, especially if someone else wants to build on your code). <---
Please write the code in a way that gives users the option to change parameters (this is mainly applicable, for example, fitting protocols/flows). In other words, please avoid hardcoding settings or physical properties.
Reasonable default values should be set, but the user needs to have the opportunity to modify them if they wish.

General code structure

We are currently aiming to follow the code structure below for each submodule (This is an initial idea; of course, this could change depending on the needs in the future)
- autoplex/submodule/job.py (any jobs defined will be inside this module)
- autoplex/submodule/flows.py (workflows defined will be hosted in this module)
- autoplex/submodule/utils.py (all functions that act as utilities for defining flow or job, for example, a small subtask to calculate some metric or plotting, will be hosted in this module)

Formatting requirements

Variable names should be descriptive and should use snake case (variable_name, not VariableName). <---
If you define a Maker, please use python class naming convention (e.g., PhononMaker, RssMaker).

Commit guidelines

pip install pre-commit.
Next, run pre-commit install (this will install all the hooks from pre-commit-config.yaml)
Step 1 and 2 needs to be done only once in the local repository
Proceed with modifying the code and adding commits as usual. This should automatically run the linters.
To manually run the pre-commit hooks on all files, just use pre-commit run --all-files
To run pre-commit on a specific file, use pre-commit run --files path/to/your/modified/module/

YuanbinLiu · 2024-08-06T12:35:08Z

@YuanbinLiu @QuantumChemist my suggestion for now would be: @QuantumChemist takes a closer look tomorrow and then both of you meet to clarify the last questions. For some reason, one can sometimes not see very well when whole code blocks have been moved to another file.

Most things have been cleared up. I will check the code to see if it still runs as intended. Still, to give my part of the approval to this PR, the following (EDIT!) three and an optional fourth criteria have to be met:

The unit tests have to be restored and adjusted to the code changes, especially if the new unit tests are not rss specific (then the rss unit tests are redundant)

a proper RSS static maker for VASP has to be written

type hints for every function.that isn't a Flow or job object (EDIT!)

separate files for the MLIP default settings (this is optional if it's not easily done)

I will temporarily set aside the final suggestion in this PR for now, as we will consider it in the upcoming modifications to MLIPFitMaker.

JaGeo · 2024-08-06T12:38:47Z

As I am on vacation, @QuantumChemist could you shortly check if everything looks good from your perspective and if you have further suggestions with regard to the three open points? Thank you!

JaGeo · 2024-08-06T12:40:56Z

@YuanbinLiu the docs are still failing. Could you fix this as well?

QuantumChemist · 2024-08-06T15:56:34Z

As I am on vacation, @QuantumChemist could you shortly check if everything looks good from your perspective and if you have further suggestions with regard to the three open points? Thank you!

This looks really good at first glance, but Yuanbin has run out of Action time. Would you prefer me to merge this PR for now and start the fixes in a new PR or should I rather merge Yuanbin's rss branch into one of my branches and push that back to this PR once the last problems are fixed?

JaGeo · 2024-08-06T16:04:29Z

I can merge but the other issues need to be addressed in a subsequent pull request in the following days. Only in this way we will avoid any issues with subsequent pull requests

JaGeo · 2024-08-06T16:17:56Z

@YuanbinLiu Could you please adress the open points in a subsequent pull request? Maybe using a fork from @QuantumChemist or @naik-aakash ? Thank you!

QuantumChemist · 2024-08-06T16:36:49Z

@YuanbinLiu Could you please adress the open points in a subsequent pull request? Maybe using a fork from @QuantumChemist or @naik-aakash ? Thank you!

I made a new branch https://github.com/QuantumChemist/autoplex/tree/rss_fixes and will start a new PR then tomorrow.

YuanbinLiu · 2024-08-07T14:50:42Z

@YuanbinLiu Could you please adress the open points in a subsequent pull request? Maybe using a fork from @QuantumChemist or @naik-aakash ? Thank you!

Yes, for sure

JaGeo · 2024-08-07T15:04:49Z

@YuanbinLiu thank you 😃

YuanbinLiu and others added 16 commits June 12, 2024 13:22

Merge branch 'JaGeo:main' into main

0e1bdbc

Update common jobs and utils, and add new RSS jobs

8a5745a

Revert "Merge branch 'JaGeo:main' into main"

12d88b5

This reverts commit 0e1bdbc, reversing changes made to 7b1012b.

Reapply "Merge branch 'JaGeo:main' into main"

df14cd9

This reverts commit 12d88b5.

Revert "add YL to contributors"

406cc82

This reverts commit 7b1012b.

added boltzhist code. WIP docstrings still need creating

c4b7144

added docstrings, updated to numpy style

9498e76

added airss installation to README

4718fb7

removed airss installation from README, will be managed by conda

05ae5f7

Merging the RSS code

bcdf0a4

resolve conflict

c267c94

Merge remote-tracking branch 'origin/main' into rss

f5efad2

Resolved merge conflicts

11a7305

passed unit tests

d4fb6df

add testing files

baae101

merging rss code

dac446b

YuanbinLiu requested review from JaGeo, QuantumChemist, vlderinger, MorrowChem, naik-aakash, dft-dutoit, jla-gardner and nfragapane July 29, 2024 10:43

YuanbinLiu changed the title ~~Integration and test of RSS code~~ [WIP] Integration and test of RSS code Jul 29, 2024

Remove redundant test files

4a69c6a

jla-gardner suggested changes Jul 30, 2024

View reviewed changes

YuanbinLiu added 2 commits July 30, 2024 16:58

fix linting errors

a34fd0c

adopt logging package

0978476

jla-gardner reviewed Jul 31, 2024

View reviewed changes

YuanbinLiu added 2 commits August 1, 2024 21:27

modify regularization test

5fdb278

Add buildcell to path on github

7ed1787

JaGeo requested changes Aug 2, 2024

View reviewed changes

QuantumChemist added 3 commits August 2, 2024 08:43

species_list is needed for the analysis plots for GAP

67e538e

species_list is needed for the analysis plots for GAP

091af95

added comment

5d3d163

QuantumChemist reviewed Aug 2, 2024

View reviewed changes

QuantumChemist added 2 commits August 2, 2024 08:56

added checks for checking if sigma regularization is active

26c609e

add docstrings

26ca71b

QuantumChemist requested changes Aug 2, 2024

View reviewed changes

QuantumChemist added 2 commits August 2, 2024 09:28

ignore airss

094d2d7

reduce the GAP unit test run time where accuracy isn't needed

4a09114

update

ba5f891

JaGeo merged commit 9413e94 into autoatml:main Aug 6, 2024


		import logging

		logging.basicConfig(level=logging.DEBUG, format='[%(levelname)s] %(message)s')

[WIP] Integration and test of RSS code #84

[WIP] Integration and test of RSS code #84

Conversation

YuanbinLiu commented Jul 29, 2024 • edited Loading

jla-gardner 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

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

QuantumChemist commented Aug 2, 2024 • edited by YuanbinLiu Loading

JaGeo commented Aug 2, 2024

JaGeo commented Aug 2, 2024

JaGeo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuantumChemist commented Aug 2, 2024

JaGeo commented Aug 2, 2024

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

QuantumChemist 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

QuantumChemist commented Aug 2, 2024 • edited Loading

QuantumChemist commented Aug 3, 2024 • edited Loading

Guidelines for contributions

General code structure

Formatting requirements

Commit guidelines

YuanbinLiu commented Aug 6, 2024

JaGeo commented Aug 6, 2024

JaGeo commented Aug 6, 2024

QuantumChemist commented Aug 6, 2024

JaGeo commented Aug 6, 2024

JaGeo commented Aug 6, 2024

QuantumChemist commented Aug 6, 2024

YuanbinLiu commented Aug 7, 2024

JaGeo commented Aug 7, 2024

YuanbinLiu commented Jul 29, 2024 •

edited

Loading

QuantumChemist commented Aug 2, 2024 •

edited by YuanbinLiu

Loading

QuantumChemist commented Aug 2, 2024 •

edited

Loading

QuantumChemist commented Aug 3, 2024 •

edited

Loading