Fix vaccum name in materials#971
Conversation
|
@gonuke in the issue you mention a solution that would be backward compatible with most model out there. Do you have an idea of the backward compatibility we main encounter ? (so I can implement a fix :) ) |
Not really... you all are the users 😀 Maybe it's not reasonable, but I just wanted to make sure it was considered. |
There was a problem hiding this comment.
Thanks for this cleanup @bam241 . I have a couple of minor questions, but otherwise it looks good.
If you want to consider this for the next release, you can update the base branch as v3.2.4-rc1. (It may require a rebase on that branch and reconciling conflicts in the ChangeLog.)
| const std::string graveyard_str_{"Graveyard"}; | ||
| const std::string vacuum_str_{"Vacuum"}; | ||
| const std::string vacuum_mat_str_{"mat:Vacuum"}; | ||
| const std::string graveyard_mat_str_{"mat:Graveyard"}; | ||
| const std::string reflecting_str_{"Reflecting"}; | ||
| const std::string white_str_{"White"}; | ||
| const std::string periodic_str_{"Periodic"}; |
There was a problem hiding this comment.
We frequently - but not always - convert these to lower case. Is there a reason to not just define them as lower case?
There was a problem hiding this comment.
I think they are only converted to lower case when comparing with a group name read from a h5 geometry.
My guess is the default if not lower case, but allowing some flexibility for the users.
I don't have preferences there, it is really up to you and Pat :)
@pshriwise as you are the initiator of the branch do you have a take on this ?
|
@pshriwise @gonuke would it be interesting to add a setter for the Allowing users to change them on the DAGMC call instead of having to change the whole geometry definition? That way we could ensure a little of backward compatibility. we could keep the default as they are now, but allow some flexibility... |
maybe @makeclean has also an opinion on my last question :) |
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
|
@gonuke I rebased against about your comment, I guess I am not the one with the most expertise about this. |
|
(btw unit test didn't run after my rebase -- Do they only run on PR vs develop ?.) |
This aims to follow up on #805.
Right now I just pulled @pshriwise branch and rebased it.
Description
This fixes the definition of the Graveyard and the Vacuum, to respectively exactly
mat:Graveyardormat:graveyardandmat:Vacuumormat:vacuum"Motivation and Context
the previous implementation would lead to detection of a vacuum where there was not, if the name of the material contained "vacuum" or "graveyard"
Changelog file
This works is sponsored by Proxima Fusion
