Add run_stats calculations and corresponding tests for FHI-aims - #1527
Add run_stats calculations and corresponding tests for FHI-aims#1527TomatoTurtlez wants to merge 2 commits into
Conversation
|
@tpurcell90 would you mind taking a look or refer to a better suited reviewer? If not, I am happy to take over but I am not an aims expert, of course! |
|
Ya I'll take a look once my laptop can log in. There send to be technical issues with the servers. |
|
@tpurcell90 sure! |
| return output_file_paths | ||
|
|
||
|
|
||
| def _parse_run_stats(aims_output_file: Path | str) -> dict[str, Any]: |
There was a problem hiding this comment.
Why is this function located here and not the pymatgen-io-aims plugin? I think it would be better for parsing of output flies to be done outside of atomate2 as much as possible.
There was a problem hiding this comment.
Hi @tpurcell90! Agreed. The current _parse_run_stats was a stopgap so I could benchmark a running job before pyfhiaims/pymatgen upstream support existed (noted in the TODO section of this PR).
I've already extended pyfhiaims.parsers (FinalMemoriesParser for peak memory, feeding a new Aims.Stdout.run_stats property) to replicate this behavior at the source, and plan to open that PR next.
Question: Would it make sense for atomate2 to depend on pyfhiaims.stdout directly, rather than going through pymatgen.io.aims.outputs.AimsOutput? That would let this land after just one upstream PR (pyfhiaims) instead of two (pyfhiaims then pymatgen).
I know the rest of the AIMS code in atomate2 currently goes through pymatgen exclusively, so I wanted to check whether that's a hard convention here or if a direct pyfhiaims dependency would be acceptable.
| re.DOTALL, | ||
| ) | ||
|
|
||
| return { |
There was a problem hiding this comment.
This will go for an entire calculation file or individual steps? This looks like it is for an entire calculation, but it would be good if this is done for each individual calculation
There was a problem hiding this comment.
Good catch. FHI-aims does print per-SCF-iteration timing/memory (End self-consistency iteration #N) as well as the SCF-initialization phase, so per-step stats are parseable.
Currently, run_stats only captures the final summary once per calculation.
I can add an opt-in flag (e.g., parse_run_stats="all" | "final" | None) to additionally capture per-iteration stats as a list, keeping the default behavior as calculation-level only so it doesn't add disk overhead for people who don't need it.
Let me know if that granularity is worth adding now or if the calculation-level summary is sufficient for this PR, and I can scope it as a fast follow-up either way.
| "tags": tags, | ||
| "completed": calcs_reversed[-1].completed, | ||
| "completed_at": calcs_reversed[-1].completed_at, | ||
| "run_stats": calcs_reversed[-1].output.run_stats, |
There was a problem hiding this comment.
I would make this an optional tag since in high-throughput this can add extra disk space usage and not everyone will need to have this information.
|
Thanks for this pull request! I made some comments, but once resolved it should be good to go |
Summary
_parse_run_statshelper toatomate2/aims/schemas/calculation.pythat parses timing, peak memory, and core-count stats directly fromaims.out(CPU/elapsed time, min/max/avg tracked memory, core count), analogous to VASP'sOutcar.run_stats.run_stats: dict[str, Any] | Nonefield toCalculationOutput(schemas/calculation.py), populated inCalculation.from_aims_filesvia_parse_run_stats.run_stats: dict[str, Any] | Nonefield toAimsTaskDoc(schemas/task.py), populated infrom_directoryfromcalcs_reversed[-1].output.run_stats— mirrors VASP's two-levelTaskDoc/CalculationOutputpattern, no re-parsing._parse_bandstructure(schemas/calculation.py) raisingAttributeError: 'AimsOutput' object has no attribute 'band_structure'wheneverparse_bandstructurewas truthy. Now guards withhasattrand warns + returnsNoneif band structure parsing isn't yet supported by the installedpymatgen.io.aims.outputs.AimsOutput.Additional dependencies introduced (if any)
None. Uses
reandpathlib.Path, both already in the standard library and already imported elsewhere in this codebase.TODO (if any)
run_statsis parsed directly fromaims.outtext in atomate2 as a stopgap. This should eventually be upstreamed sopyfhiaims.outputs.stdout.AimsStdoutandpymatgen.io.aims.outputs.AimsOutputexposerun_statsnatively (matching how VASP'sOutcar.run_statsis parsed in pymatgen, not atomate2). Once that lands,_parse_run_statshere should be removed andCalculation.from_aims_filesupdated to readaims_output.run_statsdirectly.AimsOutput.band_structure) is not implemented upstream in pymatgen. This PR only prevents the crash when it's requested; actual band structure support is out of scope here and tracked as a separate follow-up.Checklist
ruffandruff formaton your new code. This will automatically reformat your code to PEP8 conventions and fix many linting issues.