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

Check that source_bank datatype matches when reading from file #1771

Merged
merged 2 commits into from
Feb 26, 2021

Conversation

paulromano
Copy link
Contributor

Some users have been running into segfaults when trying to create a source file that is then read by OpenMC because their file did not have all the proper source site attributes (compound datatype members in the HDF5 file). Because our source file format has changed a few times recently, this isn't too surprising. This PR adds a check to ensure that the datatype members in the source file match what is expected. If they don't match, an informative error message is given.

@gridley
Copy link
Contributor

gridley commented Feb 25, 2021

Similar to how the state point files have different versions, do you think it would be good to add a version to this file? I also like the approach of just checking if the right members are there, then working as expected if they're present. If it walks like a source file and it quacks like a source file, then it must be a source file. No need to version.

BTW, for reviewing functionality like this that's not regression tested, is it expected for reviewers to pull the code and test it? I don't have any source files of different versions lying around and can't test the new functionality.

@paulromano
Copy link
Contributor Author

Yeah, that's a really good point @gridley. The nice thing about checking the attributes is that it tells you exactly what's missing, whereas if we were to do a version comparison, the error message would just be "version doesn't match" which isn't super helpful. So, I guess I'd vote to keep what's currently in this PR.

Good point on testing. It's your right as a reviewer to tell me "write a test!" Sorry for the laziness on my part -- I'll put something together.

@paulromano
Copy link
Contributor Author

Ok, just added a test for this

def test_wrong_source_attributes(run_in_tmpdir):
# Create a source file with animal attributes
source_dtype = np.dtype([
('platypus', '<f8'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent examples :0

Copy link
Contributor

@gridley gridley left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'll go ahead and merge.

@gridley gridley merged commit af22d18 into openmc-dev:develop Feb 26, 2021
@paulromano paulromano deleted the check-source-dtype branch February 26, 2021 20:51
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.

2 participants