Created a new unified flow module for RSS. #203

YuanbinLiu · 2024-11-07T17:44:23Z

No description provided.

JaGeo · 2024-11-07T18:12:44Z

'MLIP_PHONON_DEFAULTS_FILE_PATH' is not defined. This is why all workflows are failing. Could you please check this?

naik-aakash · 2024-11-07T19:38:54Z

autoplex/fitting/common/utils.py


-    Keyword Arguments
-    -----------------
+    Tunable hyperparameters


This is not recognized as a valid header in automatic doc generation, can you please revert it back as it was before?

Ah, do you mean we need to capitalise each word in the header?

Do you mean that we need to capatalise each word in the header? Or only "Keyword Arguments" is accepted?

only "Keyword Arguments" is accepted. There are ways to make other Headers to be recognized as well. It should be defined with napoleon_custom_sections in this file
https://github.com/autoatml/autoplex/blob/main/docs/conf.py

I do not have time to look into this, so if we want to have this name as header, one needs to check if this works out as intended by building docs locally with required changes in the conf.py file.

Posting here a list of recognized headers https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html#docstring-sections for your reference.

Good to know! I have changed back to the previous version.

naik-aakash · 2024-11-07T19:39:24Z

autoplex/fitting/common/utils.py


-    Keyword Arguments
-    -----------------
+    Tunable hyperparameters


Same as before

naik-aakash · 2024-11-07T19:49:45Z

autoplex/auto/rss/flow.py

+        config_file: str | None
+            Path to the configuration file that defines the setup parameters for the whole RSS workflow.
+            If not provided, the default file 'rss_default_configuration.yaml' will be used.
+        **kwargs: dict, optional


This can go under the header of Keyword Arguments in doc-strings, will make it consistent to other parts of code. See here , this is how it will be then rendered in API reference. Now I am not sure it will even get rendered properly.

https://autoatml.github.io/autoplex/reference/autoplex.fitting.common.utils.jace_fitting.html#autoplex.fitting.common.utils.jace_fitting

Sounds good!

naik-aakash · 2024-11-07T19:54:21Z

autoplex/data/common/utils.py

@@ -76,9 +81,6 @@ def rms_dict(x_ref: np.ndarray | list, x_pred: np.ndarray | list) -> dict:

    x_ref and x_pred should be of same shape.

-    Adapted and adjusted from libatoms GAP tutorial page
-    https://libatoms.github.io/GAP/gap_fitting_tutorial.html#make-simple-plots-of-the-energies-and-forces-on-the-EMT-and-GAP-datas
-
    Parameters
    ----------
    ----------1·


accidental typo? Seems not a part of this PR, but could be nice to be fixed it here itself.

Thanks for spotting this. Fixed now!

naik-aakash · 2024-11-07T20:01:25Z

autoplex/data/rss/utils.py

@@ -92,15 +115,12 @@ class HookeanRepulsion(FixConstraint):
    that the constraint will be either soft enough (e.g. non-exploding in MD) or
    strong enough (to avoid overlaps) for all spring constants and distances.

-
    Adapted from:


A general comment: We need to think of an alternative way to document this; the current method does not seem to render correctly in API documentation.

https://autoatml.github.io/autoplex/reference/autoplex.data.rss.utils.HookeanRepulsion.html#autoplex.data.rss.utils.HookeanRepulsion

@MorrowChem, do you have any ideas for this?

I just saw, References can be section header, that could be used for such cases I think.

Maybe have a look at possible header options here and see what you seem is the best fit https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html#docstring-sections

naik-aakash · 2024-11-07T20:05:22Z

autoplex/fitting/common/utils.py


-    Keyword Arguments
-    -----------------
+    Tunable hyperparameters


Should be either named as Keyword Arguments, or one needs to define a custom list of acceptable headers to have documentation properly rendered if we want to stick with Tunable hyperparameters

naik-aakash · 2024-11-07T20:06:46Z

tests/rss/test_rss.py

+    path_to_job_files = list(dir.glob("job*"))
+    for path in path_to_job_files:
+        shutil.rmtree(path)
+

 # def test_jace_rss(test_dir, memory_jobstore):


Why is this test commented out? (Again not part of this PR; just wondering now why it cannot be enabled)

This test requires the python-lammps interface, meaning that we need to have LAMMPS installed to test this module. The current setup does not include LAMMPS. Although I have successfully tested it on our cluster, I am considering waiting to test it together once we introduce the MD module. Do you have any better suggestions?

It is open source right? Then we can include it in our CI setup as well. Just let us know the version you have it tested on. I see there is option to install via conda as well. Did you use the one from Conda?

Yes, it is. But I use the standard compilation method by cmake (see https://docs.lammps.org/Build_cmake.html). I am not sure if we can use conda to install lammps and related packages (i.e., pace and python here).

There is the Conda option: https://docs.lammps.org/Install_conda.html. However, we need to ensure that it includes the PACE package (see https://docs.lammps.org/Build_extras.html#ml-pace).

Thanks, building with cmake should also be possible. I Will try to add it in another PR today or in next days in our docker image here and then this test could also be enabled. Just let me know the version you have on your systems.

naik-aakash · 2024-11-07T20:20:51Z

autoplex/data/common/flows.py

@@ -237,7 +237,7 @@ def static_run_and_convert(


 @dataclass
-class DFTStaticMaker(Maker):
+class DFTStaticLabelling(Maker):


As this maker is now renamed to DFTStaticLabelling, we would also need to rename it here on this line https://github.com/YuanbinLiu/autoplex_pub/blob/52731b1cbefd8a4546ee2b769430ff80ffb00868/autoplex/data/common/flows.py#L46

naik-aakash · 2024-11-07T20:26:54Z

autoplex/data/common/flows.py

@@ -358,13 +370,16 @@ def make(
            run_vasp_kwargs={"handlers": custom_handlers},
        )

+        if self.custom_potcar is not None:
+            st_m = update_user_potcar_settings(st_m, potcar_updates=self.custom_potcar)


this import is missing as well in this module

naik-aakash · 2024-11-07T20:28:35Z

autoplex/data/common/flows.py

            to 'bulk'. Default is None.
        """
        job_list = []

+        if isinstance(structures[0], list):
+            structures = flatten(structures, recursive=False)


flatten is not imported

naik-aakash · 2024-11-07T20:35:11Z

Hi @YuanbinLiu, I have left some minor comments. I have not looked into the code in detail (I will need more time to properly look in there; it seems quite a lot, and thanks for this PR ). These are mainly concerning issues with docstrings. Some could be addressed in this PR, mainly the header names; the rest could be addressed in another.

The rest of the comments at the end may help at least fix the failing workflows here.

naik-aakash · 2024-11-07T20:50:36Z

autoplex/fitting/common/utils.py

@@ -52,14 +52,14 @@
 )

 current_dir = Path(__file__).absolute().parent
-MLIP_PHONON_DEFAULTS_FILE_PATH = current_dir / "mlip-phonon-defaults.json"
-MLIP_RSS_DEFAULTS_FILE_PATH = current_dir / "mlip-rss-defaults.json"
+GAP_DEFAULTS_FILE_PATH = current_dir / "gap-defaults.json"


as MLIP_PHONON_DEFAULTS_FILE_PATH has been renamed now to GAP_DEFAULTS_FILE_PATH in this module, other modules where this is called also need to be changed. I found one place where it needs to be changed.

https://github.com/YuanbinLiu/autoplex_pub/blob/52731b1cbefd8a4546ee2b769430ff80ffb00868/autoplex/auto/phonons/flows.py#L10

Please also check if there are any other places where this is called.

@QuantumChemist could you help us tomorrow to figure out how we should name this in the future. Especially because of your phonon workflow. Thanks!

Yes, I've reverted the name back to the original, as I believe for potential fitting purposes, it shouldn't differentiate between RSS and phonon. The default settings for parameters should be consistent across modules. Since we plan to introduce new modules in the future, making the name more general seems appropriate. Of course, if @QuantumChemist has a better suggestion, I'd be happy to discuss more.

See #149 for further discussion

I would like to ask you to keep both files for now. The phonon workflow is finally running smoothly and I also think we might want different default settings in the future.

Based on @vlderinger 's reply here: #149 (comment), I would advocate having two separate sets of MLIP parameter files. The user is able to adjust it to their needs anyway, but as per internal default settings, we should keep it separated.

Yes, I think we reached consensus here :).

So you mean having mlip-phonon-defaults.json and mlip-rss-defaults.json? That sounds good to me for now

sorry, was late to the party 🙈

YuanbinLiu · 2024-11-08T06:28:45Z

@YuanbinLiu thank you.

'MLIP_PHONON_DEFAULTS_FILE_PATH' is not defined. This is why all workflows are failing. Could you please check this?

It is fixed now. I think it may be better to integrate the parameters of GAP and the other potentials here; otherwise, it currently looks a bit isolated. I'll set aside some time to fix this.

JaGeo · 2024-11-08T06:33:35Z

@YuanbinLiu Thanks. Yes, we would, however, need to check carefully that this doesn't change the defaults of the phonon workflow.

JaGeo · 2024-11-08T06:34:02Z

@YuanbinLiu You did not push the changes yet, btw.

YuanbinLiu · 2024-11-08T07:10:23Z

Hi @YuanbinLiu, I have left some minor comments. I have not looked into the code in detail (I will need more time to properly look in there; it seems quite a lot, and thanks for this PR ). These are mainly concerning issues with docstrings. Some could be addressed in this PR, mainly the header names; the rest could be addressed in another.

The rest of the comments at the end may help at least fix the failing workflows here.

Thank you for this. Many issues arise due to rapid updates in the code version; I'll check it again.

YuanbinLiu · 2024-11-08T07:13:15Z

@YuanbinLiu You did not push the changes yet, btw.

just pushed

YuanbinLiu · 2024-11-08T07:21:14Z

I noticed that some of my changes caused a few bugs in phonon, and I am now working to fix them.

update rss

52731b1

naik-aakash reviewed Nov 7, 2024

View reviewed changes

autoplex/fitting/common/utils.py Outdated

Keyword Arguments

-----------------

Tunable hyperparameters

Copy link

Collaborator

naik-aakash Nov 7, 2024

Choose a reason for hiding this comment

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

Same as before

naik-aakash reviewed Nov 7, 2024

View reviewed changes

fix minor issues

2067776

Created a new unified flow module for RSS. #203

Are you sure you want to change the base?

Created a new unified flow module for RSS. #203

Conversation

YuanbinLiu commented Nov 7, 2024

JaGeo commented Nov 7, 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

Choose a reason for hiding this comment

naik-aakash Nov 7, 2024 • edited Loading

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

naik-aakash Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

naik-aakash Nov 7, 2024 • edited Loading

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

naik-aakash commented Nov 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuanbinLiu commented Nov 8, 2024

JaGeo commented Nov 8, 2024

JaGeo commented Nov 8, 2024

YuanbinLiu commented Nov 8, 2024

YuanbinLiu commented Nov 8, 2024

YuanbinLiu commented Nov 8, 2024

naik-aakash Nov 7, 2024 •

edited

Loading

naik-aakash Nov 7, 2024 •

edited

Loading

naik-aakash Nov 7, 2024 •

edited

Loading