Created a new unified flow module for RSS. #203
Conversation
Update pyproject
autoplex/data/common/utils.py
Outdated
| @@ -1003,7 +1004,8 @@ def cur_select( | |||
|
|
|||
| Notes | |||
| ----- | |||
| This function calculates the descriptor vector for each atom, then performs CUR selection on the resulting vectors. | |||
| This function calculates the descriptor vector for each atom, | |||
| then performs CUR selection on the resulting vectors. | |||
|
|
|||
| Adapted from: | |||
There was a problem hiding this comment.
Use References as a header here ?
There was a problem hiding this comment.
We have adapted some code from the reference. So, would using "adapted" be better?
There was a problem hiding this comment.
See this previous comment here why Adapted won't work :
#203 (comment)
There was a problem hiding this comment.
Ah, I see. I have changed accordingly.
autoplex/data/rss/jobs.py
Outdated
|
|
||
| with open(bc_file, "w") as f: | ||
| f.writelines(contents) | ||
|
|
||
| def _is_metal(self, element_symbol): | ||
| def _is_metal(self, element_symbol: str) -> bool: |
There was a problem hiding this comment.
Wasn't it agreed to use pymatgen species for this part in your previous PR to reduce code duplication? Did something change in from #114 PR and here that we explicitly need this ?
There was a problem hiding this comment.
Thank you for the reminder. I have applied the pymatgen function in the new code.
|
|
||
| Returns | ||
| ------- | ||
| str | None |
There was a problem hiding this comment.
Maybe keep here only str as returns
And add another header Raises, and under that just mention It will raise RuntimeError when optimization fails?
There was a problem hiding this comment.
No, here we should use str | None. Since we've set a maximum number of relaxation steps, for structures far from equilibrium, even if they haven't relaxed successfully within the max steps, we can still accept them and keep rss running. When sampling, we will discard all those marked as None. Note that we often relax 10,000 structures simultaneously, so discarding some outliers won't impact the results.
|
Hi @YuanbinLiu , you can pull changes from the main branch and enable the jace_rss test now |
|
@YuanbinLiu you might have to pull the changes on main again. I had to create a new docker image. |
Ah yes. Fixed now. Thank you! |
|
*removed 😅 |
There was a problem hiding this comment.
Hey @YuanbinLiu , I have some suggestions to improve the code. I will have a look at the unit test files next.
tests/data/test_datagen_flows.py
Outdated
|
|
||
| mock_vasp(ref_paths, fake_run_vasp_kwargs) | ||
|
|
||
| job1 = DFTStaticLabelling(isolated_atom=True, |
There was a problem hiding this comment.
more desciptive job name please
tests/data/test_datagen_flows.py
Outdated
| }, | ||
| ).make(structures=test_structures) | ||
|
|
||
| job2 = collect_dft_data(vasp_dirs=job1.output) |
There was a problem hiding this comment.
here as well please
tests/data/test_datagen_jobs.py
Outdated
| from ase.io import read | ||
| from pathlib import Path | ||
| import shutil | ||
| job = RandomizedStructure(struct_number=3, |
There was a problem hiding this comment.
| job = RandomizedStructure(struct_number=3, | |
| rs_job = RandomizedStructure(struct_number=3, |
tests/data/test_datagen_jobs.py
Outdated
| 'SYMMOPS':'1-2'}, | ||
| num_processes=4).make() | ||
|
|
||
| responses = run_locally(job, ensure_success=True, create_folders=True, store=memory_jobstore) |
There was a problem hiding this comment.
| responses = run_locally(job, ensure_success=True, create_folders=True, store=memory_jobstore) | |
| responses = run_locally(rs_job, ensure_success=True, create_folders=True, store=memory_jobstore) |
tests/data/test_datagen_jobs.py
Outdated
| h2o.cell = np.ones(3)*20 | ||
| write(f'{test_dir}/data/h2o.xyz', h2o) | ||
|
|
||
| job = RandomizedStructure(struct_number=4, |
There was a problem hiding this comment.
| job = RandomizedStructure(struct_number=4, | |
| rs_job = RandomizedStructure(struct_number=4, |
tests/data/test_datagen_jobs.py
Outdated
| remove_tmp_files=True, | ||
| num_processes=4).make() | ||
|
|
||
| _ = run_locally(job, ensure_success=True, create_folders=True, store=memory_jobstore) |
There was a problem hiding this comment.
| _ = run_locally(job, ensure_success=True, create_folders=True, store=memory_jobstore) | |
| run_locally(rs_job, ensure_success=True, create_folders=True, store=memory_jobstore) |
tests/data/test_datagen_jobs.py
Outdated
| num_processes=4).make() | ||
|
|
||
| responses = run_locally(job, ensure_success=True, create_folders=True, store=memory_jobstore) | ||
| assert len(read(job.output.resolve(memory_jobstore), index=":")) == 3 |
There was a problem hiding this comment.
| assert len(read(job.output.resolve(memory_jobstore), index=":")) == 3 | |
| assert len(read(rs_job.output.resolve(memory_jobstore), index=":")) == 3 |
tests/data/test_datagen_jobs.py
Outdated
| num_processes=4).make() | ||
|
|
||
| _ = run_locally(job, ensure_success=True, create_folders=True, store=memory_jobstore) | ||
| ats = read(job.output.resolve(memory_jobstore), index=":") |
There was a problem hiding this comment.
| ats = read(job.output.resolve(memory_jobstore), index=":") | |
| ats = read(rs_job.output.resolve(memory_jobstore), index=":") |
tests/data/test_datagen_jobs.py
Outdated
| bcur_params=bcur_params, | ||
| random_seed=None).make() | ||
|
|
||
| job = Flow(generate_structure, output=generate_structure.output) |
There was a problem hiding this comment.
same suggestion for more descriptive job names for all of the other job names as well
| @@ -409,11 +411,10 @@ def test_mlip_fit_maker_with_automated_separated_dataset( | |||
| # Test if gap fit runs with pre_database_dir | |||
| gapfit = MLIPFitMaker().make( | |||
| species_list=["Li", "Cl"], | |||
| isolated_atoms_energy=[-0.28649227, -0.25638457], | |||
There was a problem hiding this comment.
gapfit takes the isolated atoms energy from the separate IsoAtom calculation entry in the xyz file directly.
The unit tests have been renamed. |
I think you didn't push it ? |
just pushed. I also fixed an issue with a unit test for j-ace. |
|
Hi @JaGeo , @QuantumChemist , please wait untill test durations file is updated |
I don't plan to merge anything without @JaGeo explicitly asking me to do so 😶🌫️ |
|
The only last general issue left now (that has to be addressed in a new PR) is the coverage for this particular file here |
|
Yes, @QuantumChemist . Please see #223 😉 |
Okidoki 👍🏻 |
A new flow (named
RssMaker) has been created to make it easy to set up and run RSS, which can be found inautoplex/auto/rss/flows.py.