-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding methods for graveyard removal and creation #714
Conversation
src/dagmc/util.hpp
Outdated
|
||
#include <string> | ||
|
||
namespace moab { // TODO: separate into a new namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not creating right away a dagmc_tool
namespace ?
f979f1e
to
ac88714
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
I really like the exaustive tests implemented !
Some few comments on implementation details, nothing major tho !
thx @pshriwise !
src/dagmc/DagMC.cpp
Outdated
// there should not be more than one graveyard group | ||
if (graveyard_count > 1) { | ||
MB_CHK_SET_ERR(MB_FAILURE, | ||
"More than one graveyard group is present in the model"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as it is a failure and we don't return how much there is, is it worth to continue looping after founding the second graveyard ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is after the loop, which does go over all groups to count the number of graveyards found. A good point that we don't return how many graveyard groups were found though -- I've added that to the error message here.
src/dagmc/DagMC.cpp
Outdated
sets_to_delete.merge(graveyard_vols); | ||
|
||
// get the implicit complement, it's children will need updating | ||
EntityHandle ic = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if ic
is a commun shortcut for implicit complement, but maybe it is worse expending it a bit ? icomplement
? or ..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
ic
is one we've used frequently in the past, but there's no reason not to be more explicit here. I'll update that
// report an error | ||
if (has_graveyard() && !overwrite) { | ||
MB_CHK_SET_ERR(MB_FAILURE, "Graveyard already exists"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this can be combined together ?
if (overwrite) {}
else if(has_graveyard()) {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow... this error should only be raised in the case that a graveyard already exists and the overwrite flag is set to false. Separating the two would have a different meaning than the one that exists here.
That being said, the check for !overwrite
isn't really necessary, as the block above removes the graveyard if overwrite
is true
, meaning that at this point if a graveyard already exists we can raise an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just line 392 you had a
if(overwrite){}
and l 398
if (has_greaveyard() && !overwrite){}
my suggestion was to combine the two into a if() else if()
but your new implementation resolves it :)
src/dagmc/DagMC.cpp
Outdated
double bump = 10 * numerical_precision(); | ||
box.upper[i] += bump; | ||
box.lower[i] -= bump; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the 2 loop to increase the size of of the box could be part of a dedicated method ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mayb as part of the BOX
struc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Good suggestion. Better to put something like that in the struct for use elsewhere.
src/dagmc/DagMC.cpp
Outdated
return rval; | ||
} | ||
|
||
ErrorCode DagMC::box_to_surf(double llc[3], double urc[3], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const &
for the double[3]
arguments ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it were a data structure that could get really large, then yes we'd want to do that. I don't think it's worth it here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Making them const is probably good for peace of mind though :)
src/dagmc/util.hpp
Outdated
|
||
#include <string> | ||
|
||
namespace moab { // TODO: separate into a new namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Already mentionned it... but adding it here for simplicity) why not already introduce a dagmc_tool
namespace ?
src/dagmc/dagmcmetadata.cpp
Outdated
for (int i = 0; i < input.size(); i++) { | ||
input[i] = std::tolower(input[i]); | ||
} | ||
moab::lowercase_str(input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the use of the moab
namespace here because one can believe this comes from MOAB where it is actually a dagmc method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I don't disagree, but this happens a lot in DAGMC. In fact, the entire
DagMC
class is in themoab
namespace. I was mostly trying to follow suit here. As an incremental step, I'll update this to adagmc_util
namespace. There might be an even better approach here though. I'll think on it a bit.
I think I've touched on all of your comments at this point @bam241. Thanks for your review! I think it's ready for another round. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this @pshriwise !
This looks good to me.
I'll merge by the end of the week if no-one argues :)
Please wait to merge. This doesn't quite work with double-down yet -- it just isn't reflected in the checks here b/c we don't test against double-down for PRs. |
@pshriwise for safety I converted this into a draft. so it won't be merged... |
@pshriwise - we're gearing up for a release and you tagged this for a 3.2.1 release (we'll probably jump to 3.3.0). Do you want to work this in? |
Yeah, I'd like to if possible. I'll rebase it and push now. |
230a3a5
to
aac7811
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops - I thought I'd submitted this review already...
doc/CHANGELOG.rst
Outdated
* DagMC::remove_graveyard | ||
* DagMC::create_graveyard | ||
* DagMC::has_graveyard | ||
* DagMC::get_graveyard_group | ||
* DagMC::category_tag | ||
* DagMC::box_to_surf (private method) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we need all this detail in the CHANGELOG
Just resolved a few simple conflicts |
…ercase characters in place.
… the instance when entities are deleted.
…d the model limits.
I'm a little confused by the last commit... Does @shimwell have write permission on @pshriwise fork? |
e1998a9
to
f8db99a
Compare
I don't think so? @shimwell submitted a PR to update this branch handling some merge conflicts. I didn't see it in time and ended up doing my own rebase (Sorry, Jon). |
Ah yes sorry just trying to help by clicking the solve conflicts button on this thread and resolving the conflicts (which I guess submits a PR against the fork branch).Sorry for the confusion. |
Thanks @pshriwise |
Description
This PR adds the following public methods related to graveyard management:
DagMC::remove_graveyard
DagMC::create_graveyard
DagMC::has_graveyard
DagMC::get_graveyard_group
DagMC::category_tag
We may want to have a discussion about moving some of these capabilities into the
GeomTopoTool
class in MOAB. If that's the case, that's fine by me but I figured this PR contains the framework and a place to have that discussion 😃Motivation and Context
These capabilities were added so that models can be exported from Cubit/Trelis with or without a graveyard. The new API functions allow external codes to automatically generate a graveyard volume if one is not present on the model. It also allows an external application to remove the graveyard if needed.
In addition to reducing workflow overhead, the ability to work with models that don't contain a graveyard makes visualization much more convenient.
Other Information
This PR has a few byproducts that come with it:
dagmcMetaData::to_lower
method now relies on a common call to thelowercase_str
method which is provided in the newly addedutil.hpp
file. Thelowercase_str
method has been added as part of themoab
namespace to avoid populating the common namespace (though we may want to have a discussion about our overall namespace situation sometime in the future).DagMC
class. This is purely used as a convenience for calculating the global bounding box of the problem when creating a graveyard.DagMC::category_tag
method. This adds some MOAB-specific syntax to DAGMC, but I wasn't sure how else to handle it and it's needed to find the graveyard volume.pincell.h5m
, that was created in Trelis to ensure that theremove_graveyard
method works without leaving orphaned entities or entity sets behind.Resolves #649