Add properties to settings w/ documentation, c++ loading of filename, and python round-trip test

[gonuke]

Description

This adds the path of the properties.h5 file as a new entry in the simulation settings. This file path can be used for loading properties from a file other than the default.

This will replace #3807 that has been withdrawn.

Fixes # (issue)

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[lewisgross1296]

Looking at discussion on #3807, I think I agree with Patrick that we should have a properties sub-element of settings with it's own subelements that should be booleans of whether to load temperatures, densities, and perhaps something like check_mats (default to true). I don't expect most people to be loading materials from the properties file, it's just exists as a check to make sure you're not loading properties from some model that doesn't really match your current one. We are the special case where our properties.h5 specifically does not include the zoned materials we make after generating properties. I think I agree there should also be a filepath subelement of properties that defaults to properties.h5 but can be changed to something else.

[gonuke]

Working on this - ran out of time figuring out the XML stuff for this - stay tuned

[lewisgross1296]

I like the settings approach, probably best to have it in one place, as opposed to every cell that has properties.

Might be a future issue/PR, but we should discuss how to handle if there are cells with instances in the properties file that are not distribcell in our current model. (e.g. programmatically added tallies from Cardinal)

[gonuke]

Hmmm... seems like my version of clang-format is too new and it is aligning comments differently that v15 (although the style guide does call for aligning comments)

[lewisgross1296]

Recently, I've been installing clang-format-15 inside my docker image and formatting from the command line via clang-format-15 -i FILE

[lewisgross1296]

Looked around / thinking about testing. It seems like this is the only OpenMC test that uses properties.h5.

Our tests could also take advantage of lib_init and export properties files with temperatures only/densities only/both. We could use the lattice_distribmat and lattice_distribrho tests as a base for the models that will have properties.

I can imagine the following tests being useful (and maybe more?)

1. import a property file that has both temperature/density and ask for both. Some kind of check that the cells have the temps we expect.
2. import a property file that only has temperature, ask for density, and test for the error
3. import a property file that has only density, ask for temperature, and test for the error
4. ask for temperature and density from a file that has neither, and test for both errors
5. ask for properties when no properties file exists

[gonuke]

Recently, I've been installing clang-format-15 inside my docker image and formatting from the command line via clang-format-15 -i FILE

That's what I get for trying to do this on my Mac :)

[gonuke]

@pshriwise - we thought this would be nice to have, but I've just realized that returning an error code - as we may want to do - is not compatible with the function signature. Interested in design guidance:
a) skip this and just test for valid file after this call
b) figure out how to make this function do all the things.

I'm inclined to (a) for now and let this be a separate issue down the road.

[pshriwise]

Seems like you landed where I did on this. It's probably still useful in some form, but isn't high priority and isn't worth the distraction in this PR. 👍🏻

[gonuke]

It looks like I need to figure out how to place a dummy properties.h5 file in the tmpdir to test this - or back off ever further on testing this, noting that the existence of other files is not tested in unit tests

[pshriwise]

It looks like I need to figure out how to place a dummy properties.h5 file in the tmpdir to test this - or back off ever further on testing this, noting that the existence of other files is not tested in unit tests

This context managet may be of help

openmc/tests/__init__.py

Lines 8 to 25 in 83a30f6

	def cdtemp(files=None):
	"""Context manager to change to/return from a tmpdir.
	Parameters
	----------
	files : Iterable of str or Path-like
	Set of files to copy into the temporary directory
	"""
	with tempfile.TemporaryDirectory() as tmpdir:
	cwd = os.getcwd()
	if files:
	for file in files:
	copy(file, tmpdir, follow_symlinks=True)
	try:
	os.chdir(tmpdir)
	yield
	finally:
	os.chdir(cwd)

[gonuke]

It looks like I need to figure out how to place a dummy properties.h5 file in the tmpdir to test this - or back off ever further on testing this, noting that the existence of other files is not tested in unit tests

This context manager may be of help

Perhaps, but it's already using a temp directory fixture. I could use both but not sure how they interact. I could also just use os to touch a file in the current temp dir.

The fixture is a little mysterious because it refers to a tmpdir but I can't find anywhere that tmpdir is defined??

[pshriwise]

Oh, I misunderstood. I thought you were trying to reference an already-present properties file. I don't quite understand why we need the file to exist though, it looks like that's only checked on the C++ side.

I think comparison of the str vs. Path types might be an issue. The value passed to the Settings.properties_file property is converted to a Path object in the call to input_path (if it isn't one already).

[gonuke]

I think comparison of the str vs. Path types might be an issue. The value passed to the Settings.properties_file property is converted to a Path object in the call to input_path (if it isn't one already).

Yeah - that's probably the problem. I was getting wrapped around on the type of the filename and not sure what will resolve to true.

[pshriwise]

Thanks @gonuke! I've included some minor comments here.

My comment on load order can be tackled in a separate PR.

[pshriwise]

Not critical but a slight style preference here for readability

Suggested change

	if (read_densities_from_properties) {
	hid_t material_group = open_group(group, "material " + std::to_string(id_));
	double density;
	read_attribute(material_group, "atom_density", density);
	this->set_density(density, "atom/b-cm");
	close_group(material_group);
	}
	if (!read_densities_from_properties) return;
	hid_t material_group = open_group(group, "material " + std::to_string(id_));
	double density;
	read_attribute(material_group, "atom_density", density);
	this->set_density(density, "atom/b-cm");
	close_group(material_group);

[gonuke]

I have this lingering desire to have a single point of return from functions. I don't remember where in my past I got that bias... I've fixed this per your suggestion, though.

[pshriwise]

Commenting on what I think may be a pre-existing issue here and looking for guidance:

Above, we call Cell::set_density which populates the density multiplier vector based on the density of the material filling that cell. Here, we read the density of the material from the file, which may change the baseline density of the material from what it was at the time we called Cell::set_density and possibly invalidate the density multipliers of the cells? In a separate effort, should we perhaps change the order of these imports?

Looping @nuclearkevin in for a take as well.

[gonuke]

A simple change to mitigate (but not entirely prevent) this is to have two read_density flags: read_cell_densities and read_material_densities, and document that one will override the other.

[pshriwise]

My preference would be to either read all domain densities or none and fix the ordering (again, in a separate PR if need be). My understanding is that some cells may have density multipliers and others may not, so the additional option could leave us in a confusing state where some densities are original and some from the properties file.

[gonuke]

I misunderstood your early comment. I thought you were worried about one of them overwriting the other, but I see that the concern is more subtle.

[lewisgross1296]

This is perhaps outside the scope of this PR and can be handled in a separate issue/PR, but is this the place where we'd have to safeguard against distribcell offsets not in our model that exist in the properties file?

I think that there's a possibility that a cell in model::cells does not have distribcell offsets in the model but does have them in the properties file. I'm not sure we should skip those cells because they might have a temperature we want but it also might error if we try to load them (e.g. vector range check 0 error message)

[gonuke]

I'll call it outside the scope for now. It seems like something best debugged and improved once this fundamental capability has been implemented.

[lewisgross1296]

I think it's working!

 Importing properties from
 /home/lgross/cnerg/gcmr/multiphysics_depletion/BOL_mp_depl/180/properties.h5...
[openmc.deplete] t=0.0 s, dt=86400 s, source=3333333.3333333335

[gonuke]

I can't quite figure out why this test is failing? It seems like one of the python version tests was cancelled and that caused CI to be determined as failed. Can someone relaunch tests?