svalinn/DAGMC

False positive for assignment of vacuum

pshriwise opened this issue · 7 comments

pshriwise commented

@shimwell recently sent me an input with material assignments like "lower_vaccum_vessel", which DAGMC interpreted as a vaccuum assignment. This is caused by the following lines:

DAGMC/src/dagmc/dagmcmetadata.cpp

Lines 209 to 212 in 50caabf

bool is_graveyard =
to_lower(grp_name).find(to_lower(graveyard_str)) != std::string::npos;
bool is_vacuum =
to_lower(grp_name).find(to_lower(vacuum_str)) != std::string::npos;

Prior to cc505bb, this would not have been a problem since only the presence of "Vacuum" (as opposed to "vacuum") in the material assignment would have triggered this behavior. This is still what I would call a bug, the corner case was just rarer.

I think we either need to document that "V/vacuum" should not be used in material names or only assign volumes a vacuum condition if the lowercase version of the name matches "mat:vacuum" exactly instead of searching for the presence of "vacuum", which is what happens in the lines linked above.

The word vacuum starts to look very strange after typing it so many times at this time of night.

shimwell commented

Oh wow, I thought we squashed this bug a while back but I see how we didn't quite catch all the use cases. Looks like this is the difference between the current DAGMC used in OpenMC implementation and the version Shift implementation you are noticing @jbae11

I remembered not to call tags mat:vacuum. But I didn't recall the fix searched for the string vacuum in the entire material tag. Good to know, I can rename my tags.

Plus one vote for searching for a string that is equal to =="mat: vacuum" instead of finding vacuum in the string.

Material tags that would become a vacuum: 😀

mat:vacuum_vessel
mat:lid_of_vacuum_vessel
mat:not_a_vacuum
mat:partial_vacuum
mat:dyson_vacuum_cleaner
mat:area_needs_vacuuming

pshriwise commented

I tend to agree, @shimwell -- I vote we use an exact match for those assignments.

@makeclean @gonuke any thoughts/feelings here?

makeclean commented
pshriwise commented

It's kind of a regression in that materials containing a lower-case "vacuum" trigger now also trigger this problem, yes, but I think the more general problem is that we're searching for "vacuum" instead of using an exact match. For example, use of the material name mat:VacuumVessel would also have caused that volume to be assigned a void/vacuum material before the switch to lowercase string comparisons.

I'm confident that the problem is in DAGMC proper, not the OpenMC interface. I have a DAGMC branch to this issue where an exact match is used and it fixed the problem in @shimwell's OpenMC simulation.

  • 👍1
gonuke commented

Looking back, this flaw has always been in this code. I'm all for the change you propose. Would it make sense to change vacuum_str to simply be mat:vacuum and then also change for a match rather than a find?

pshriwise commented

I think vacuum_str is used in a couple of other places without the "mat:" bit. How about a new member variable, vacuum_mat_str?

gonuke commented

I wasn't sure if the other uses were semantically different... ? or could deal with the redefined string.

Also - similar code exists in the mcnp_funcs.cpp interface and it might be good to update that for consistency. (or have the MCNP code rely on this??)

  • 👍2