openmc-dev
mgxs_lib.domains does not preserve order
When we define mgxs_lib.domains = user_defined_material_list, I noticed in my own research last summer that the order is not always conserved. I believe it is because mgxs_lib.domains converts user_defined_material_list to an unordered list in openmc.mgxs.library.
When a user calls create_mg_mode, they often do it like this, by explicitly passing the domain names in the same order as the user_defined_material_list: mgxs_lib.create_mg_mode(xsdata_names=[‘uo2’, ‘zirc’, ‘water’])
It is sometimes a faulty assumption to list the xsdata_names in the same order as user_defined_material_list.
@pshriwise and I have been discussing possible fixes. One of the options is to make changes in create_mg_mode that defaults to using the domain names, if provided in the original domain definition, rather than passing the xsdata_names since the user has no idea what order they might be in within the function.
@nelsonag might also be interested in this topic.
Thanks @mkreher13! This is a good problem to solve, no doubt.
I think there are two problems at once here (clarify as needed):
- The domain order is not preserved.
- It is a PITA to name regions with a procedurally-generated naming scheme.
I think the way item 1 can show up is if using Library.domains = "all" or passing in the result of something like object.get_all_[materials, cells, universes, material_cells]().values() when using Python < 3.6 (where dictionary order was not preserved). Otherwise, I don't see where in the Library class the order could be changed after setting Library.domains. Am I understanding that right or did you come across another scenario?
For item 2, you're approach is not unreasonable, but the problem is that the xsdata objects must have name uniqueness and there is no way to guarantee that for domain names. A check could be done afterwards, and identifiers appended (e.g., a domain named "UO2" becomes "UO2_2" if "UO2" already exists in the MGXSLibrary), but doesnt really seem like an improvement over the current "set1", "set2", .... I also admit that I can't see how this would play out with distribcells at the moment. I am 100% in support of a solution here, I just cant think of a nice, clear, and generalizable one.
@nelsonag yeah I thnk those are two likely scenarios where one would run into this problem.
And agreed for the case of the distribcell and mesh domains. For meshes, the sheer number of cells makes it difficult to generate meaningful names (maybe mesh indices?). Same for the distribcell domains. Maybe we just don't allow custom names for those for now?
For the material, cell, or universe domains, I think a flag requesting that the name attribute of the domain be used could work with the requirements you mentioned:
- all of the domain objects have a name assigned
- names o the domain objects are unique
There's the dictionary approach to consider also, which is maybe more flexible, but it also seems a little redundant as we already have name attributes for these domains.
It seems that the name uniqueness could be handled with an on-the-fly check for name uniqueness as the xs file is generated, adding names to a set and ensuring that the size of the set increases as new names are added. If we find that the names of the objects are not unique, we'd raise an error and request that the user correct the names or remove the flag before calling that method again.
I don't have as much experience with these workflows as either of you, @nelsonag and @mkreher13, so maybe this is all more trouble than it's worth. :shrug:
As a user, when I first noticed the problem, I made a new order of my domain names manually, then I passed that new list as xs_data_names:
names = []
for mat in mgxs_lib.domains: names.append(mat.name)
mgxs_file = mgxs_lib.create_mg_library(xs_type=”macro”, xs_data_names=names)
I think this is a reasonable fix that any user can figure out. Perhaps just adding a warning when a user specifies xs_data_names when calling create_mg_library is sufficient. I agree that a more sophisticated fix on the OpenMC side may be too difficult considering how many different domain types there are (if it was easy, I might have just fixed it myself in a PR, as I have done with other mgxs issues).
By the way, I just reproduced an example with python3.9. The materials are exported in this order: fuel, zircaloy, water1, water2, water3. But when filling cells with materials, there was no order. First I made a water3 tube, then I made a fuel cell, etc.
Then: mgxs_lib.domains = geometry.get_all_materials().values() Then: print("DOMAINS: ", mgxs_lib.domains)
The order of the print statement showed a completely unpredictable order: [fuel, zircaloy, water1, water3, water2]. Without knowing this order ahead of time, I can't reliably pass xs_data_names without manually re-ordering.
@mkreher13 Yeah, I can't say for now why exactly the order was what you had, but i think in general the usage of get_all_materials() will lead to a cumbersome/difficult approach for the users (geometry.get_all_materials().values iterates through the levels of the root universe to get all the materials with a depth-first search, which sure, is algorithmic, but not certainly not simply understood)
So, while we dont have a guarantee of name uniqueness, we do have ID uniqueness. If we replaced the default of set1, set2, ... with <mat_id>_<domain_id>{_subdomain_id}" (where {} indicates present as needed) then it wont be human readable, but will be fully reproducible. I think that is better than the set<#>`` approach, but since the user may not be manually setting IDs it still is not ideal.
I like the idea to order them by mat_id. They can still be called set1, set2, etc, but have the numbers reflect the material id.
In the current system, the user really has a hard time figuring out which multi-group cross sections correspond to which domain (even when setting names with xs_data_names since the order isn't clear). So moving to an ID numbering scheme sounds more robust overall.