Parse metadata exception handling #112
Conversation
gonuke
left a comment
gonuke
left a comment
There was a problem hiding this comment.
Thanks @Waqar-ukaea - I have a few questions here about the choice of defaults
src/mesh_manager_interface.cpp
Outdated
| return surface_metadata_.at({surface, type}); | ||
| } | ||
| catch (const std::out_of_range e) { | ||
| return VOID_MATERIAL; |
There was a problem hiding this comment.
This already suggests that the transmission boundary condition should be the default. A surface property is never a material.
Although I wonder if this is a good choice for default if/when other properties get applied to surfaces?
There was a problem hiding this comment.
I'm not really certain what the best default for this would be so I'm going to leave it as is for now. Maybe @pshriwise has a better idea of what we should do here.
There was a problem hiding this comment.
For this I'd recommend a transmission boundary as @gonuke said.
return {PropertyType::BOUNDARY_CONDITION, "transmission"};There was a problem hiding this comment.
Although I wonder if this is a good choice for default if/when other properties get applied to surfaces?
Can you expand on what you mean here @gonuke?
There was a problem hiding this comment.
In theory, surfaces could get tagged with metadata for purposes other than boundary conditions, e.g. some variance reduction parameters, etc. In such a case, a default that represents a boundary condition may not be the best default.
src/mesh_manager_interface.cpp
Outdated
| return {PropertyType::BOUNDARY_CONDITION, "transmission"}; | ||
| return surface_metadata_.at({surface, type}); | ||
| } | ||
| catch (const std::out_of_range e) { |
There was a problem hiding this comment.
Doesn't the count check above make this already more robust? Can you add tests that fail before this change and pass afterwards?
There was a problem hiding this comment.
Also, could this count approach be used for volume properties as well, instead of the exception?
There was a problem hiding this comment.
Agreed on this one too. Added a similar count check for get_volume_property()
|
Looking at the
|
|
There is probably a wider conversation to be had about how exactly we want to be handling exceptions within XDG. I went with |
3 participants
PR for #111