feat: add get_fcidump to output#253
Open
haneug wants to merge 4 commits into
Open
Conversation
haneug
added
enhancement
haneug
force-pushed
the
feature/output-fcidump-parser
branch
from
a2b39ef to
17a2832
Compare
timmyte
requested changes
Jun 25, 2026
| """ | ||
| Parse the fcidump file generated by ORCA and return its data in the `Fcidump` data class. | ||
| The fcidump file has to be generated by the ORCA job and cannot be generated on-the-fly after the calculation. | ||
| ORCA can generate fcidump files via the `dumpactints true` flag in the `%output` block. |
Contributor
There was a problem hiding this comment.
Suggested change
| ORCA can generate fcidump files via the `dumpactints true` flag in the `%output` block. | |
| To generate FCIDUMP files, set the following options: | |
| ``` | |
| %output | |
| dumpactints true | |
| end | |
| ``` |
IMO this reads more concise.
|
|
||
| def get_fcidump(self) -> Fcidump | None: | ||
| """ | ||
| Parse the fcidump file generated by ORCA and return its data in the `Fcidump` data class. |
Contributor
There was a problem hiding this comment.
Suggested change
| Parse the fcidump file generated by ORCA and return its data in the `Fcidump` data class. | |
| Parse the FCIDUMP file generated by ORCA and return its data in the `Fcidump` data class. |
| def get_fcidump(self) -> Fcidump | None: | ||
| """ | ||
| Parse the fcidump file generated by ORCA and return its data in the `Fcidump` data class. | ||
| The fcidump file has to be generated by the ORCA job and cannot be generated on-the-fly after the calculation. |
Contributor
There was a problem hiding this comment.
Suggested change
| The fcidump file has to be generated by the ORCA job and cannot be generated on-the-fly after the calculation. | |
| The FCIDUMP file has to be generated by the ORCA job and cannot be generated on-the-fly after the calculation. |
| Returns | ||
| ------- | ||
| fcidump_data: Fcidump | None | ||
| The parsed fcidump data or None if the file is not present or could not be parsed. |
Contributor
There was a problem hiding this comment.
Suggested change
| The parsed fcidump data or None if the file is not present or could not be parsed. | |
| The parsed FCIDUMP data or None if the file is not present or could not be parsed. |
| @@ -0,0 +1,161 @@ | |||
| """Parse a potential FCIDUMP file""" | |||
Contributor
There was a problem hiding this comment.
A module that contains one primary class, should also be named after that class -> fcidump.py
| return tensor | ||
|
|
||
| @classmethod | ||
| def parse_fcidump(cls, path: Path | str) -> "Fcidump": |
Contributor
There was a problem hiding this comment.
- IMO
from_file()is a more accessible name thanparse_fcidump(). - Are the FCIDUMP files documented anywhere? Could add the link to the docstring, if so.
| @classmethod | ||
| def _get_int(cls, key: str, header: str) -> int: | ||
| """Return the integer value of the given key.""" | ||
| m = re.search(rf"{key}\s*=\s*(\d+)", header, re.IGNORECASE) |
Contributor
There was a problem hiding this comment.
What about negative numbers?
If these lines always look as follows:
key = 1234 [END OF LINE]
You could use .partition("=")
| @classmethod | ||
| def _get_int_list(cls, key: str, header: str) -> list[int]: | ||
| """Return a list of integers corresponding to the given key.""" | ||
| m = re.search(rf"{key}\s*=\s*([\d,\s]+)", header, re.IGNORECASE) |
Contributor
There was a problem hiding this comment.
Suggested change
| m = re.search(rf"{key}\s*=\s*([\d,\s]+)", header, re.IGNORECASE) | |
| m = re.search(rf"{key}\s*=\s*(\d+(\s*,\s*\d+)+)", header, re.IGNORECASE) |
What about a more precise regex!?
This still does not account for leading plus/minus symbol.
| @@ -0,0 +1,79 @@ | |||
| import textwrap | |||
Contributor
There was a problem hiding this comment.
Please add units test for _get_int_list() and _get_int() specifically.
|
|
||
| @pytest.mark.unit | ||
| def test_parse_fcidump_header(tmp_path: Path) -> None: | ||
| fcidump_text = textwrap.dedent("""\ |
Contributor
There was a problem hiding this comment.
I would actually not remove the indentation.
If a file format does not rely on indentation, then our file parser shouldn't as well. And the current also implementation does (good implementation 👍)
So I would actively test that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes Issues
Closes None
Description
output.get_fcidump.Release Notes
Added
get_fcidumpfor easy access to fcidump file properties.