False positive for assignment of vacuum

[pshriwise]

@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]

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]

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

@makeclean @gonuke any thoughts/feelings here?

[makeclean]

Seems like a regression that was fixed long ago, is the bug in dagmc proper or the alternative arrangements in OpenMC? Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Patrick Shriwise ***@***.***> Sent: Wednesday, April 20, 2022 2:48:22 PM To: svalinn/DAGMC ***@***.***> Cc: Davis, Andrew ***@***.***>; Mention ***@***.***> Subject: Re: [svalinn/DAGMC] False positive for assignment of vacuum (Issue #805) I tend to agree, @shimwell<https://github.com/shimwell> -- I vote we use an exact match for those assignments. @makeclean<https://github.com/makeclean> @gonuke<https://github.com/gonuke> any thoughts/feelings here? — Reply to this email directly, view it on GitHub<#805 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AASTUSTUYSJMUAQR2S43IC3VGADKNANCNFSM5T2U5QAA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[pshriwise]

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.

[gonuke]

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]

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]

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??)

[jon-proximafusion]

I think this is the bug that got you today @rherrero-pf

[gonuke]

Hmmm... Looking back on the conversation from 2.5 years ago, I guess I was hoping for a PR to be launched to address this. 😀

Anyone game to suggest a fix? Ideally one that is sufficiently backwards compatible with most models we expect to be out there.