openmc-dev / openmc

OpenMC Monte Carlo Code
https://docs.openmc.org
Other
753 stars 484 forks source link

S(a,b) name suggestions instead of applying a guess #1945

Closed pshriwise closed 2 years ago

pshriwise commented 2 years ago

The openmc.thermal.get_thermal_name function has a feature that guesses the name of a thermal library using Python's builtin difflib.get_close_matches function. This is useful as some names are rather particular (like JEFF, for instance). However, it can lead to situations where unintended S(a,b) tables are applied. For example, a user recently tried to apply a custom S(a,b) table to a material but because the new name they gave the table ("c_Graphite_20p") wasn't present in the _THERMAL_NAMES dictionary the name "c_Graphite_30p" was applied instead. Thankfully the user examined the resulting materials.xml file and caught the problem.

To avoid this in the future, I'll suggest that we require an exact name match and that the guesses found by get_close_matches be reported in a raised ValueError from get_thermal_name. This provides the user information on how to fix their problem along with suggestions for existing names in the case that they've made small typo.

paulromano commented 2 years ago

There is a way (or at least supposed to be a way) to override the name:

openmc.data.ThermalScattering(neutron_file, thermal_file, table_name='c_Graphite_20', zaids=[6000, 6012, 6013], nmix=1)

The extra table_name argument avoids that whole guessing business. However, notice that you have to specify three arguments: table_name, zaids, and nmix. The ENDF evaluations unfortunately don't provide all the information you need to process them into ACE files, so the guessing we do also provides, in addition to a standard name, this extra information. Namely, zaids is the list of ZAID identifiers that should be associated with the S(α,β) table and nmix is the number of atom types in a mixed moderator. If you want to provide your own table name, the deal is that OpenMC won't guess the other information for you, so you need to provide it.

Now, this all being said, it appears there is currently a bug such that the above line doesn't quite work and the name gets overridden at the very end. I have a branch in progress with some bug fixes, so I'll add a fix there and submit a PR shortly.

pshriwise commented 2 years ago

Very interesting @paulromano! I didn't know that about the ENDF evaluations, but then there's a lot I don't know about the nuclear data world 😄

After looking at #1948 and re-reading the forum post again, I think the root of the problem here is that we always search openmc.data.therma._THERMAL_NAMES in material.add_s_alpha_beta.

This is problematic in the situation that a user is trying to apply a custom dataset they've created with their own thermal scattering data using custom names. If the new name is close enough to an existing name, it's very possible that an unintended S(a,b) data will be applied to that material if the user doesn't catch that a guess has been applied (as we see happening in the discourse thread linked above). It also appears that even if the name is different enough to get past the _THERMAL_NAMES search that "c_" will be prepended to the original name, which will likely cause a mismatch in the user's custom data when trying to read the S(a,b) information in the hdf5 data library at runtime.

As for solutions, I first considered registering the new name in _THERMAL_NAMES upon creation of a new openmc.data.thermal.ThermalScattering object, but that will only last as long as the current Python instance is active, meaning that a user would always have to do library generation and material specification in the same instance for it to work. Providing an option to skip the get_thermal_names search in material.add_s_alpha_beta seems like a simple, reasonable solution to me. Overall, my biggest concern is that we might silently apply the wrong data when the user is requesting something else.

gridley commented 2 years ago

As a response to the short-lived registration of a new name, might I dare suggest we use some .conf/openmc files? It could be some group of XML (or whatever) based settings that are persistent through any separate python/C++ runs.

Aside from thermal scattering data, the things previously set as environment variables could go into such a file. I don't know of any other things this would be useful for, though. I love having configurable dotfiles for other programs, though.

pshriwise commented 2 years ago

I'm not opposed to that idea @gridley. In the case that a user is doing work on multiple machines, they'd have to manage their configuration files in multiple places accordingly. I guess I'd be in favor of supporting some kind of configuration files, but for operations like the one in this issue I'd prefer if we didn't require them.

I'm going to close this as it's been resolved by one of @paulromano's recent PRs. Want to continue the discussion in a new issue on the topic?

pshriwise commented 2 years ago

Resolved by #1948.