Merging RSS code #69

YuanbinLiu · 2024-06-12T15:47:37Z

This PR is mainly for merging RSS code.

Todo list

Refactor autoplex/data/common/utils.py into smaller files with informative names (e.g. selection.py)
Numpy docstrings for all functions
Write tests for RSS generation and selection
Full support for multicomponent generation and selection
Add buildcell installation similar to github workflow (we need to use the latest buildcell version)

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

This reverts commit 12d88b5.

This reverts commit 7b1012b.

JaGeo · 2024-06-12T16:36:45Z

Great. We need to find a way to run the tests. I think all action time is used up here as well.

naik-aakash · 2024-06-13T04:37:05Z

It seems not the action time, but some refactoring is done, and imports fail, leading to failures in all tests and linting issues. It is probably not yet ready for review and is still a work in progress. I guess

JaGeo · 2024-06-13T05:05:57Z

Ah, I thought i had used it all for the last pull request.

Before I start with the review all tests need to pass and we again need tests for the new modules.

JaGeo · 2024-06-13T05:07:54Z

autoplex/data/common/utils.py

@@ -783,3 +783,85 @@ def plot_energy_forces(
    )
    plt.savefig(train_name.replace("train", "energy_forces").replace(".extxyz", ".png"))
    plt.show()
+
+
+class Species:


A part of this should be covered by pymatgen and ase. Why is a new class needed here? @YuanbinLiu

It provides some new functionalities. For example, return the unique atomic pairs in a dataset according to the specific format.

I have seen these two parts but might be worth checking ase/pymatgen if some of the other functionality is already covered. I am hesistant to reinvent the wheel as it means more work in maintaining and testing code ;).

naik-aakash · 2024-06-13T05:25:32Z

autoplex/data/rss/jobs.py

+            self._cell_seed(self.buildcell_options, self.tag)
+            bt_file = '{}.cell'.format(self.tag)
+
+        with Pool() as pool:


Better to limit number of cpu used here, provide it as an arg ?

naik-aakash · 2024-06-13T07:40:09Z

autoplex/data/rss/jobs.py

+
+        tmp_file_name = "tmp." + str(i) + '.' + tag + '.cell'
+
+        run("buildcell",


From what I found so far, this is a separate binary that needs to be compiled before one can use it. We also need this to be installed for tests to run successfully for this maker. We would need to add additional steps in the GitHub CI workflow to get this installed before tests, as we did for Julia.

@YuanbinLiu Is this how you installed it on your system?
https://airss-docs.github.io/getting-started/installation/

I did not find any pip installable package or precompiled binary available.

There is one package named airsspy, but it seems more like a wrapper and does not install buildcell.

Could you check if buildcell is used in pymatgen? I remember something like rhis

I think I am wrong. ... We might need to compile it during the test setup... but we surely need to check that everything works. If we cannot get it to run here, other people will surely struggle as well.

buildcell is very straightforward to compile, unlike many fortran things, and is central to RSS, so I think we ought to invest the effort in this

@MorrowChem Does the conda version work? https://anaconda.org/conda-forge/airss-with-default-names

And, yes, we absolutely need to have this as a part of our test framework. This is clear.

Yes, the website instruction is what I am using for the installation of buildcell. I have noticed airsspy before. But I find there is still some difference between them. So agree with Joe's opinion here. @naik-aakash

works for me (on linux, not mac), so agreed we should go with the conda version

I don't remember where exactly in the tutorial it was but maybe we can incorporate some steps from @MorrowChem 's tutorial here https://github.com/MorrowChem/how-to-validate-potentials to set up buildcell.

JaGeo · 2024-06-13T08:08:23Z

autoplex/data/common/utils.py

@@ -1035,7 +1118,7 @@ def boltzhist_CUR(atoms,
    else: 
        selected_atoms = selected_bolt_ats

-    ase.io.write('boltzhist_CUR.extxyz', selected_atoms, parallel=False)
+    ase.io.write('boltzhist_CUR.extxyz', selected_atoms, parallel=False) # TODO: should we be writing random files like this?


Is this info important. One can always access these files within joblow. If it is very important, we might want to have the info in the overall output

But as we do the latter aleady... Do we need this for restarts?

I sometimes use it for the structural analysis (e.g., see if the generated structures make sense). But it indeed does not affect the whole workflow. Will take it out.

It can be useful to have this for debugging if it is easier like this. We can then leave it.

We also generate phonopy.yaml files as they might be easier to reuse than the database.

JaGeo · 2024-06-13T08:45:43Z

@MorrowChem can you try installinf buildcell via the workflow to have everything ready for creating automatic tests?

YuanbinLiu · 2024-06-13T13:58:50Z

It seems not the action time, but some refactoring is done, and imports fail, leading to failures in all tests and linting issues. It is probably not yet ready for review and is still a work in progress. I guess

Ah, I thought i had used it all for the last pull request.

Before I start with the review all tests need to pass and we again need tests for the new modules.

For sure. It is still in progress. I estimate it will take at least one week to pass all tests on our (Joe's and my) side so that we could be able to do the final test on github.

YuanbinLiu · 2024-06-18T14:49:27Z

How about renaming the job "generate_randomized_structures" to "generate_distorted_structures"? I realized that it could be confused with the random structures generated by buildcell. The "generate_randomized_structures" job doesn't create completely random structures but rather makes some perturbations. Therefore, to differentiate between the two, it might be better to rename it to "generate_distorted_structures". Would this be feasible? @QuantumChemist @naik-aakash

JaGeo · 2024-06-18T14:57:47Z

How about renaming the job "generate_randomized_structures" to "generate_distorted_structures"? I realized that it could be confused with the random structures generated by buildcell. The "generate_randomized_structures" job doesn't create completely random structures but rather makes some perturbations. Therefore, to differentiate between the two, it might be better to rename it to "generate_distorted_structures". Would this be feasible? @QuantumChemist @naik-aakash

"generate_rattled_structures" maybe? And the other ones, we call random structures? I don't think distorted captures what we are doing. (@YuanbinLiu )

JaGeo · 2024-06-18T15:00:23Z

Or displaced structures?

YuanbinLiu · 2024-06-18T15:02:00Z

How about renaming the job "generate_randomized_structures" to "generate_distorted_structures"? I realized that it could be confused with the random structures generated by buildcell. The "generate_randomized_structures" job doesn't create completely random structures but rather makes some perturbations. Therefore, to differentiate between the two, it might be better to rename it to "generate_distorted_structures". Would this be feasible? @QuantumChemist @naik-aakash

"generate_rattled_structures" maybe? And the other ones, we call random structures? I don't think distorted captures what we are doing.

ah, "generate_rattled_structures" sounds great!

naik-aakash · 2024-06-19T07:03:56Z

Hi @YuanbinLiu, Yeah, renaming it to "generate_rattled_structures" sounds good to differentiate between the two methods better.

QuantumChemist · 2024-06-19T10:14:49Z

Hi @YuanbinLiu, Yeah, renaming it to "generate_rattled_structures" sounds good to differentiate between the two methods better.

I agree.

JaGeo · 2024-06-21T11:52:19Z

@QuantumChemist maybe we can both check the code for an intermediate review.

The unit tests don't seem to run at the moment. Not sure why. Likely action time.

QuantumChemist · 2024-06-21T12:02:41Z

@QuantumChemist maybe we can both check the code for an intermediate review.

The unit tests don't seem to run at the moment. Not sure why. Likely action time.

I could check it locally or make a new branch in Aakash's repo to check the unit tests.

naik-aakash · 2024-06-21T13:06:04Z

@QuantumChemist maybe we can both check the code for an intermediate review.

The unit tests don't seem to run at the moment. Not sure why. Likely action time.

Hi @JaGeo , I have disabled it on the repo. As I suspected, there will be multiple commits and action time might get wasted in the process.

naik-aakash · 2024-06-21T13:09:11Z

Also, from what I can see, buildcell is not added yet to github ci workflow. So, tests will fail anyway, which depends on that particular program

naik-aakash · 2024-06-21T13:15:39Z

@QuantumChemist maybe we can both check the code for an intermediate review.
The unit tests don't seem to run at the moment. Not sure why. Likely action time.

I could check it locally or make a new branch in Aakash's repo to check the unit tests.

Maybe testing locally is good idea.

JaGeo · 2024-06-21T13:24:07Z

Also, from what I can see, buildcell is not added yet to github ci workflow. So, tests will fail anyway, which depends on that particular program

Ah. Okay. There is an Installation instruction in the new readme file.

JaGeo · 2024-07-08T08:32:53Z

I will try to get the buildcell installed on the repo so that @YuanbinLiu can continue with this pull request more easily.

JaGeo · 2024-07-16T20:18:05Z

Buildcell will now be automatically installed on the repo.

JaGeo · 2024-07-29T06:50:31Z

Can you please revert this, @YuanbinLiu ?

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

Merge branch 'JaGeo:main' into main

0e1bdbc

Update common jobs and utils, and add new RSS jobs

8a5745a

YuanbinLiu requested review from JaGeo, QuantumChemist, vlderinger, MorrowChem, naik-aakash and nfragapane June 12, 2024 15:47

YuanbinLiu added 3 commits June 12, 2024 16:59

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.

JaGeo reviewed Jun 13, 2024

View reviewed changes

naik-aakash reviewed Jun 13, 2024

View reviewed changes

JaGeo changed the title ~~Merging RSS code~~ [WIP] Merging RSS code Jun 13, 2024

added boltzhist code. WIP docstrings still need creating

c4b7144

naik-aakash reviewed Jun 13, 2024

View reviewed changes

MorrowChem added 2 commits June 13, 2024 08:48

added docstrings, updated to numpy style

9498e76

added airss installation to README

4718fb7

JaGeo reviewed Jun 13, 2024

View reviewed changes

removed airss installation from README, will be managed by conda

05ae5f7

QuantumChemist mentioned this pull request Jun 20, 2024

Docstrings and documentation #65

Merged

9 tasks

Merging the RSS code

bcdf0a4

JaGeo mentioned this pull request Jul 8, 2024

test switching to conda #79

Closed

JaGeo mentioned this pull request Jul 16, 2024

installation of buildcell #81

Merged

YuanbinLiu added 3 commits July 24, 2024 13:32

resolve conflict

c267c94

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

f5efad2

Resolved merge conflicts

11a7305

YuanbinLiu merged commit 11a7305 into autoatml:main Jul 29, 2024

YuanbinLiu changed the title ~~[WIP] Merging RSS code~~ Merging RSS code Jul 29, 2024


		tmp_file_name = "tmp." + str(i) + '.' + tag + '.cell'

		run("buildcell",

Merging RSS code #69

Merging RSS code #69

Conversation

YuanbinLiu commented Jun 12, 2024 • edited by JaGeo Loading

JaGeo commented Jun 12, 2024

naik-aakash commented Jun 13, 2024 • edited Loading

JaGeo commented Jun 13, 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

JaGeo Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

YuanbinLiu Jun 13, 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

JaGeo commented Jun 13, 2024

YuanbinLiu commented Jun 13, 2024

YuanbinLiu commented Jun 18, 2024

JaGeo commented Jun 18, 2024 • edited Loading

JaGeo commented Jun 18, 2024

YuanbinLiu commented Jun 18, 2024

naik-aakash commented Jun 19, 2024

QuantumChemist commented Jun 19, 2024

JaGeo commented Jun 21, 2024

QuantumChemist commented Jun 21, 2024

naik-aakash commented Jun 21, 2024

naik-aakash commented Jun 21, 2024

naik-aakash commented Jun 21, 2024

JaGeo commented Jun 21, 2024

JaGeo commented Jul 8, 2024

JaGeo commented Jul 16, 2024

JaGeo commented Jul 29, 2024

YuanbinLiu commented Jun 12, 2024 •

edited by JaGeo

Loading

naik-aakash commented Jun 13, 2024 •

edited

Loading

JaGeo Jun 13, 2024 •

edited

Loading

YuanbinLiu Jun 13, 2024 •

edited

Loading

JaGeo commented Jun 18, 2024 •

edited

Loading