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

Improve parsing of the ADF15 metadata for H-like ions. #425

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

vsnever
Copy link
Member

@vsnever vsnever commented Nov 23, 2023

This PR addresses the issue raised in #424. Some ADAS ADF15 (PEC) data files for hydrogen-like ions actually have "hydrogen" and not "hydrogen-like" format of the metadata.

While the user can specify the header format in install_adf15() and parse_adf15(), the parse_adf15() can also try to parse the metadata in "hydrogen" format, if parsing in "hydrogen-like" format fails and the header_format is not specified by the user.

Also, if parsing of the ADF15 metadata fails, parse_adf15() does not raise an error and install_adf15() runs silently without updating the repository. This behaviour is confusing to the user and should be fixed, which is done in this PR.

Comment on lines +79 to +81
if not config:
raise RuntimeError("Unable to parse ADF15 metadata.")

Copy link
Member

Choose a reason for hiding this comment

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

I like this. More obvious failure modes are good.

Comment on lines 74 to 75
if not config: # try hydrogen header (works for 'bnd' files)
config = _scrape_metadata_hydrogen(file, element, charge)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a risk of this inadvertently parsing a different header file incorrectly if it fails on the hydrogen-like parser? I think it would be better to explicitly handle the 'bnd' files here as a special case (with a comment about why it's a special case), and for anything else let it fall through and error until we can manually verify that the same workaround can be applied.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not found any files in ADAS other than those with the "bnd" suffix affected by this issue. Now I did an explicit check on the file name, but since ADAS may update the "bnd" files in the future, the parser tries to read the metadata in "hydrogen-like" format first, and if that fails, then for "bnd" files, it reads the metadata in "hydrogen" format.

@jacklovell jacklovell merged commit cd06ea0 into cherab:development Dec 1, 2023
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Module OpenADAS.impact_excitation_pec raises RuntimeError 'Requested PEC rate is not available'
2 participants