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

Database load has a hidden exception #2054

Open
john-science opened this issue Jan 13, 2025 · 2 comments
Open

Database load has a hidden exception #2054

john-science opened this issue Jan 13, 2025 · 2 comments
Labels
cleanup Code/comment cleanup: Low Priority

Comments

@john-science
Copy link
Member

john-science commented Jan 13, 2025

A bare except with a pass inside is always a bit troublesome, but this is an important piece of code:

try:
cs.loadFromString(
self.h5db["inputs/settings"].asstr()[()], handleInvalids=handleInvalids
)
except KeyError:
# not all paths to writing a database require inputs to be written to the
# database. Technically, settings do affect some of the behavior of database
# reading, so not having the settings that made the reactor that went into
# the database is not ideal. However, this isn't the right place to crash
# into it. Ideally, there would be not way to not have the settings in the
# database (force writing in writeToDB), or to make reading invariant to
# settings.
pass

According to Nick below, the original reasons for this are no longer relevant. So I recommend removing this (and testing to make sure everything still works).

@john-science john-science added the cleanup Code/comment cleanup: Low Priority label Jan 13, 2025
@john-science john-science changed the title Database write has a hidden exception Database load has a hidden exception Jan 13, 2025
@ntouran
Copy link
Member

ntouran commented Jan 14, 2025

I think it's from a transition time when we first started writing inputs to the databases. We wanted it to still work if the inputs weren't in there but also work if the inputs were in there. This is obviously not a good solution, and it's probably not a meaningful issue anymore unless we want to dig up old results. You can probably just remove the try/except safely here.

@john-science
Copy link
Member Author

Thanks, Nick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code/comment cleanup: Low Priority
Projects
None yet
Development

No branches or pull requests

2 participants