Skip to content

Conversation

@mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented Nov 24, 2025

To be reviewed after #454 is merged


Refactor a bunch of hdf5 file-reading functionality.

  • This addresses one of the items from Tabulated UVB/cooling needs refactoring #128.
  • Overall, we are now a lot more "careful" (i.e. we verify that a lot of related properties are the same rather than just assume that they are the same)

The biggest change here is that I factored out the parsing of the attributes of the tabulated interpolation datasets. This is now handled by parse_GridTableProps and returns an object that summarizes the information

  • the cost to this is that we are allocating some extra memory (this shouldn't be a huge deal since this is all done at initialization)
  • a major benefit is that we could easily choose to do the following (we don't actually do any of this in this PR):
    • we could validate that the names of quantities along axes are are what we expect (e.g. "hdens", "redshift", "Temperature", "metal * log")
    • we could validate that different datasets have consistent attributes
    • I think that this validation is something we should be doing, but it will introduce slightly more overhead (and I didn't want to do it before talking it over)
  • Realistically, the main reason I did this is so I can reuse the machinery when it comes time to parse the datafiles I made holding the multi-species dust information1.

Footnotes

  1. Honestly, if I had to do it all over again, I would do it slightly differently. This was a lot more effort than I expected. But I think it is a very useful improvement for us to have.

@mabruzzo mabruzzo added the refactor internal reorganization or code simplification with no behavior changes label Nov 24, 2025
Comment on lines +199 to +211
// Ideally we would uncomment the following block of logic that verifies
// that the Grid Table properties are identical to the cooling table
// -> uncommenting the logic would be useful for enforcing consistency
// in any new data files (all files currently shipped with Grackle
// already satisfy this requirement)
// -> the only downside to uncommenting the block is a little overhead

//if (h5io::assert_has_consistent_GridTableProps(file_id, dset_name,
// grid_props) != GR_SUCCESS){
// h5io::drop_GridTableProps(&grid_props);
// H5Fclose(file_id);
// return GR_FAIL;
//}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would really like to make this change. The only drawback is that it will introduce slight overhead at startup. But, I think it's probably worth it

Suggested change
// Ideally we would uncomment the following block of logic that verifies
// that the Grid Table properties are identical to the cooling table
// -> uncommenting the logic would be useful for enforcing consistency
// in any new data files (all files currently shipped with Grackle
// already satisfy this requirement)
// -> the only downside to uncommenting the block is a little overhead
//if (h5io::assert_has_consistent_GridTableProps(file_id, dset_name,
// grid_props) != GR_SUCCESS){
// h5io::drop_GridTableProps(&grid_props);
// H5Fclose(file_id);
// return GR_FAIL;
//}
// validate that Heating table has have identical GridTableProps
if (h5io::assert_has_consistent_GridTableProps(file_id, dset_name,
grid_props) != GR_SUCCESS){
h5io::drop_GridTableProps(&grid_props);
H5Fclose(file_id);
return GR_FAIL;
}

Comment on lines +226 to +238
// Ideally we would uncomment the following block of logic that verifies
// that the Grid Table properties are identical to the cooling table
// -> uncommenting the logic would be useful for enforcing consistency
// in any new data files (all files currently shipped with Grackle
// already satisfy this requirement)
// -> the only downside to uncommenting the block is a little overhead

//if (h5io::assert_has_consistent_GridTableProps(file_id, mmw_dset_name,
// grid_props) != GR_SUCCESS){
// h5io::drop_GridTableProps(&grid_props);
// H5Fclose(file_id);
// return GR_FAIL;
//}
Copy link
Collaborator Author

@mabruzzo mabruzzo Dec 4, 2025

Choose a reason for hiding this comment

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

Just like the preceding comment, I would really like to make this change

Suggested change
// Ideally we would uncomment the following block of logic that verifies
// that the Grid Table properties are identical to the cooling table
// -> uncommenting the logic would be useful for enforcing consistency
// in any new data files (all files currently shipped with Grackle
// already satisfy this requirement)
// -> the only downside to uncommenting the block is a little overhead
//if (h5io::assert_has_consistent_GridTableProps(file_id, mmw_dset_name,
// grid_props) != GR_SUCCESS){
// h5io::drop_GridTableProps(&grid_props);
// H5Fclose(file_id);
// return GR_FAIL;
//}
// validate that MMW table has have identical GridTableProps
if (h5io::assert_has_consistent_GridTableProps(file_id, mmw_dset_name,
grid_props) != GR_SUCCESS){
h5io::drop_GridTableProps(&grid_props);
H5Fclose(file_id);
return GR_FAIL;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor internal reorganization or code simplification with no behavior changes

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant