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

Implementation of adaptive supercell settings and improvement of the documentation #80

Merged
merged 59 commits into from
Aug 27, 2024

Conversation

QuantumChemist
Copy link
Collaborator

@QuantumChemist QuantumChemist commented Jul 11, 2024

Implemented adaptive settings for the supercells and improved the documentation.

The PR is in principle ready to be merged. 😃

I also added

  • making data_preprocessing continue even if not all data calculations were successfull. Making it abort the workflow if the percantance is below 95%

in this PR and have to fix the unit tests accordingly once the atomate2 VASP input issues have been fixed.

@QuantumChemist QuantumChemist added documentation Improvements or additions to documentation enhancement New feature or request labels Jul 11, 2024
@QuantumChemist QuantumChemist requested a review from JaGeo July 11, 2024 20:01
jobs.append(supercell_job)
supercell_matrix = supercell_job.output
except AttributeError:
supercell_matrix = generate_supercell_matrix(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@JaGeo what do you think about keeping it as an "if everything else fails" option? To keep the workflow from aborting. As it's really annoying if most of your materials are running fine, but it's just one or two structures where the settings didn't match that well.

structure, supercell_matrix
)
warnings.warn(
message="Falling back to a simple supercell size schema."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with a warning that tells user to check their data in case this happened?

phonon_displacement_maker = TightDFTStaticMakerBigSupercells()
if adaptive_phonopy_supercell_settings:
lattice_avg = sum(structure.lattice.abc) / 3
if lattice_avg > 10:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Schon bei 10?

Copy link
Collaborator Author

@QuantumChemist QuantumChemist Jul 31, 2024

Choose a reason for hiding this comment

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

yes, if the lattice parameters of the unit cell have been larger than 10 on average (!), it created those 1000+ atoms supercells with the usual supercell size settings that are hard to get converge. One example is mp-971662, where we arrive at 1242 atoms with a 3x3x3 supercell matrix. It has lattice parameters a = b = c = 10.18 in the primitive unit cell.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't you just use the supercell size itself to determine the maker?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The 3x3x3 supercell matrix was just an example here. But using min_length=18 created a similar big supercell. The CubicSupercellTransformation functions allows for up to 1200 atoms in the prefer_90_degrees=True case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case I can't use the supercell size because the determination of the supercell only happens within the atomate2 PhononMaker. The only way to control it is by using min_length. I could add a get_supercell_size call maybe?

Copy link
Collaborator

@JaGeo JaGeo Jul 31, 2024

Choose a reason for hiding this comment

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

and without force_90_degree and the orthorhombic option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm working on a unit test to check this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also subsequently make the requirement smaller: i.e, test 13 Angstrom or 10 as a min_length

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or even iterate over it in steps of 1 or 2 Angstrom, maybe until 10 angstrom

Copy link
Collaborator Author

@QuantumChemist QuantumChemist Jul 31, 2024

Choose a reason for hiding this comment

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

mp-971662 was not among the troublesome phonopy supercells (but the rattled ones), but mp-1200830 was among the materials that caused this issue, the lattice parameter of the primitive cell is a = b = c = 10.76. I'm going to push the unit test now.

)

assert new_matrix == expected_matrix
Copy link
Collaborator Author

@QuantumChemist QuantumChemist Jul 31, 2024

Choose a reason for hiding this comment

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

@JaGeo
when I run this unit test, I get

============================== 1 passed in 15.04s ==============================
PASSED                       [100%]/home/certural/miniconda3/envs/auto_debug/lib/python3.10/site-packages/_pytest/python.py:159: UserWarning: Falling back to orthorhombic, prefer 90.
  result = testfunction(**testargs)
/home/certural/miniconda3/envs/auto_debug/lib/python3.10/site-packages/_pytest/python.py:159: UserWarning: Falling back to orthorhombic.
  result = testfunction(**testargs)
/home/certural/miniconda3/envs/auto_debug/lib/python3.10/site-packages/_pytest/python.py:159: UserWarning: Falling back to a simple supercell size schema. Check if this is ok for your use case.
  result = testfunction(**testargs)

Process finished with exit code 0

(you have to scroll sideways to see the warnings)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my actual calculation for mp-1200830, I had

supercell_matrix:
- [  -2,  -2,   0 ]
- [  -2,   0,  -2 ]
- [   0,  -2,  -2 ]

It created a supercell with 656 atoms, which I could only converge with reduced settings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I will then do the following:
start with cubic, then as you suggested above using allow_orthorhombic=True and prefer_90_degrees=False and then also reduce min_length as for the phonopy supercells, those are the only elements, I can control the supercell size with

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But my question would still be how would that influence the DFT phonon calculation quality, if usually a min_length of around 20 is recommended? That's why I opted for reducing the calculation accuracy depending on the average unit cell lattice parameter

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it will reduce the quality but if the k point density is roughly stable, it should not be too bad.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright! :)

@QuantumChemist
Copy link
Collaborator Author

QuantumChemist commented Aug 26, 2024

Hey @JaGeo , what is left now for this PR is that I fix all the atomate2 related VASP errors in the unit tests.


matrix = np.array([[a_factor, 0, 0], [0, b_factor, 0], [0, 0, c_factor]])
return matrix.transpose().tolist()
# despite the warning Expected type 'list', got 'object' instead, this is of <class 'list'>
Copy link
Collaborator Author

@QuantumChemist QuantumChemist Aug 26, 2024

Choose a reason for hiding this comment

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

and here I checked very carefully, that it really returns a list (it does). I couldn't find out why PyCharm flags it as object, so I left it like that. I don't think this will lead to code inconsistencies or bugs.

@QuantumChemist
Copy link
Collaborator Author

Hey @JaGeo 👋🏻
so the unit test run before my last push was sucessful, but then I noticed an issue with database_dir which I also fixed now. But I ran out of action time. I'm not sure if you planned to merge this before I tested a few supercells on supermuc, but in case you want to merge it, shall I repeat the unit test wf for the last push in e.g. Aakash's repo or is it ok like this?

Comment on lines +178 to 189
# this will only run if train.extxyz and test.extxyz files are present in the database_dir
mlip_fit_job = machine_learning_fit(
database_dir=database_dir,
isolated_atoms_energies=isolated_atoms_energies,
num_processes_fit=num_processes_fit,
auto_delta=auto_delta,
glue_xml=glue_xml,
mlip_type=self.mlip_type,
HPO=self.HPO,
hyper_param_optimization=self.hyper_param_optimization,
ref_energy_name=self.ref_energy_name,
ref_force_name=self.ref_force_name,
ref_virial_name=self.ref_virial_name,
Copy link
Collaborator Author

@QuantumChemist QuantumChemist Aug 27, 2024

Choose a reason for hiding this comment

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

Hey @YuanbinLiu,
I have another question. I noticed that this part will only run successfully, when there is a train.extxyz and test.extyz file present (which are usually generated in the data_prep step), and I guess that this is the case for when you do the fit with the RSS-generated data. Is that correct?

@JaGeo
Copy link
Collaborator

JaGeo commented Aug 27, 2024

@QuantumChemist I will merge it here. If a unit test fails, I assume you will take care of it :).

@JaGeo JaGeo merged commit f4fbdf2 into autoatml:main Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants