Adding methods for graveyard removal and creation by pshriwise · Pull Request #714 · svalinn/DAGMC

pshriwise · 2020-11-30T06:00:05Z

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:

the dagmcMetaData::to_lower method now relies on a common call to the lowercase_str method which is provided in the newly added util.hpp file. The lowercase_str method has been added as part of the moab namespace to avoid populating the common namespace (though we may want to have a discussion about our overall namespace situation sometime in the future).
the addition of a BBOX struct to the DagMC class. This is purely used as a convenience for calculating the global bounding box of the problem when creating a graveyard.
the addition of a 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.
this adds a test file of a pincell, pincell.h5m, that was created in Trelis to ensure that the remove_graveyard method works without leaving orphaned entities or entity sets behind.

Resolves #649

bam241 · 2021-01-20T23:34:44Z

+
+#include <string>
+
+namespace moab { // TODO: separate into a new namespace


why not creating right away a dagmc_tool namespace ?

bam241

Looking good.
I really like the exaustive tests implemented !
Some few comments on implementation details, nothing major tho !
thx @pshriwise !

bam241 · 2021-01-21T16:53:12Z

+  // 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");
+  }


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 ?

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.

bam241 · 2021-01-21T16:58:20Z

+  sets_to_delete.merge(graveyard_vols);
+
+  // get the implicit complement, it's children will need updating
+  EntityHandle ic = 0;


not sure if ic is a commun shortcut for implicit complement, but maybe it is worse expending it a bit ? icomplement ? or ..?

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

bam241 · 2021-01-21T17:01:28Z

+  // report an error
+  if (has_graveyard() && !overwrite) {
+    MB_CHK_SET_ERR(MB_FAILURE, "Graveyard already exists");
+  }


maybe this can be combined together ?

if (overwrite) {} else if(has_graveyard()) {}

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.

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

bam241 · 2021-01-21T17:05:12Z

+    double bump = 10 * numerical_precision();
+    box.upper[i] += bump;
+    box.lower[i] -= bump;
+  }


maybe the 2 loop to increase the size of of the box could be part of a dedicated method ?

mayb as part of the BOX struc ?

Good suggestion. Better to put something like that in the struct for use elsewhere.

bam241 · 2021-01-21T17:05:53Z

+  return rval;
+}
+
+ErrorCode DagMC::box_to_surf(double llc[3], double urc[3],


const & for the double[3] arguments ?

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.

Making them const is probably good for peace of mind though :)

bam241 · 2021-01-21T17:16:20Z

+
+#include <string>
+
+namespace moab {  // TODO: separate into a new namespace


(Already mentionned it... but adding it here for simplicity) why not already introduce a dagmc_tool namespace ?

bam241 · 2021-01-21T17:17:05Z

-  for (int i = 0; i < input.size(); i++) {
-    input[i] = std::tolower(input[i]);
-  }
+  moab::lowercase_str(input);


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.

I don't disagree, but this happens a lot in DAGMC. In fact, the entire DagMC class is in the moab namespace. I was mostly trying to follow suit here. As an incremental step, I'll update this to a dagmc_util namespace. There might be an even better approach here though. I'll think on it a bit.

pshriwise · 2021-01-24T15:58:08Z

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.

bam241

Thank you for this @pshriwise !
This looks good to me.

I'll merge by the end of the week if no-one argues :)

pshriwise · 2021-01-26T18:24:54Z

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.

bam241 · 2021-01-26T18:48:38Z

@pshriwise for safety I converted this into a draft. so it won't be merged...

gonuke · 2021-07-23T18:18:12Z

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

pshriwise · 2021-07-23T19:04:20Z

Yeah, I'd like to if possible. I'll rebase it and push now.

gonuke

oops - I thought I'd submitted this review already...

gonuke · 2021-07-23T20:29:38Z

+     * DagMC::remove_graveyard
+     * DagMC::create_graveyard
+     * DagMC::has_graveyard
+     * DagMC::get_graveyard_group
+     * DagMC::category_tag
+     * DagMC::box_to_surf (private method)


Not sure we need all this detail in the CHANGELOG

shimwell · 2021-08-17T15:37:28Z

Just resolved a few simple conflicts

…instance.

…ercase characters in place.

… the instance when entities are deleted.

…d the model limits.

gonuke · 2021-08-17T16:05:07Z

I'm a little confused by the last commit... Does @shimwell have write permission on @pshriwise fork?

pshriwise · 2021-08-17T16:08:19Z

I'm a little confused by the last commit... Does @shimwell have write permission on @pshriwise fork?

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

shimwell · 2021-08-17T16:54:12Z

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.

gonuke · 2021-08-17T22:01:43Z

Thanks @pshriwise

pshriwise changed the title ~~Adding Functions for graveyard removal and creation~~ Adding methods for graveyard removal and creation Nov 30, 2020

pshriwise requested review from bam241 and ljacobson64 and removed request for ljacobson64 November 30, 2020 19:06

pshriwise added the 3.2.1 Issues and PRs targeted for the 3.2.1 release label Jan 18, 2021

bam241 reviewed Jan 20, 2021

View reviewed changes

pshriwise force-pushed the rm_mk_graveyard branch from f979f1e to ac88714 Compare January 21, 2021 16:49

bam241 requested changes Jan 21, 2021

View reviewed changes

bam241 approved these changes Jan 26, 2021

View reviewed changes

bam241 marked this pull request as draft January 26, 2021 18:48

pshriwise mentioned this pull request Mar 23, 2021

Add double-down job to CI for pull requests #733

Closed

pshriwise force-pushed the rm_mk_graveyard branch from 230a3a5 to aac7811 Compare July 23, 2021 19:17

gonuke added the Next Release label Jul 23, 2021

pshriwise marked this pull request as ready for review July 23, 2021 21:39

gonuke requested changes Aug 13, 2021

View reviewed changes

shimwell mentioned this pull request Aug 14, 2021

utils function remove_tag_from_h5m_file is untested fusion-energy/openmc-dagmc-wrapper#33

Closed

pshriwise added 7 commits August 17, 2021 10:58

Adding creation of interior graveyard surface triangles.

a3a1164

Moving INFTY symbol into the MOAB namespace.

ee09934

Adding a check for consistency when creating volumes.

cba9b1a

A whole bunch of reorganization.

18ef758

Adding new file for testing graveyard creation/removal.

20f2eb0

Factoring out a function for retrieving the group sets from the MOAB …

3f8bd24

…instance.

Adding a utility header and function for transforming a string to low…

882d57e

…ercase characters in place.

pshriwise added 22 commits August 17, 2021 11:00

A much more simple version of graveyard entity collection.

0b8c4cc

Re-adding removal of volume trees.

a8ca3d5

Accumlating and deleting sets separately from entities.

516a085

Adding a bunch of test conditions for the Trelis file.

9177422

No need to remove parent/child relationships. Those are cleaned up in…

b08b39b

… the instance when entities are deleted.

Applying style guide.

7eb68e1

Updating some comments for clarity in the graveyard test file.

0e340de

Removing unecessary empty line.

06add3e

Some comment updates for clarity. Reducing the size of the bump beyon…

ddd3af3

…d the model limits.

Adding some brief documentation to the BBOX class.

e60b35d

Placing the method in the moab namespace.

e6b8833

Adding news file.

b5c2296

Applying style guide.

aacbafc

ic to implicit_complement

2267b72

Simplifying graveyard presence check.

ef6946d

Adding an expand method to the box struct for convenience.

0ea45ed

Making array args to box_to_surf const.

95f1f8d

Moving lowercase_str into a new dagmc_util namespace.

364c24d

Report number of graveyard groups to user in error message.

1134648

Adding some wrapper functions to avoid propagation of ifdefs.

67a4765

Removing old news file. Updating changelog.

a940fb5

Removing duplicate getobb definition after rebase.

f8db99a

pshriwise force-pushed the rm_mk_graveyard branch from e1998a9 to f8db99a Compare August 17, 2021 16:05

Applying style guide.

5b418ec

Removing unecessary detail in changelog.

c31cf5d

gonuke merged commit 9deb4e7 into svalinn:develop Aug 17, 2021


		#include <string>

		namespace moab { // TODO: separate into a new namespace

Conversation

pshriwise commented Nov 30, 2020 • edited by gonuke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Other Information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bam241 left a comment

Choose a reason for hiding this comment

Uh oh!

bam241 Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshriwise Jan 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshriwise Jan 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshriwise Jan 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshriwise Jan 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pshriwise commented Jan 24, 2021

Uh oh!

bam241 left a comment

Choose a reason for hiding this comment

Uh oh!

pshriwise commented Jan 26, 2021

Uh oh!

bam241 commented Jan 26, 2021

Uh oh!

gonuke commented Jul 23, 2021

Uh oh!

pshriwise commented Jul 23, 2021

Uh oh!

gonuke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimwell commented Aug 17, 2021

Uh oh!

gonuke commented Aug 17, 2021

Uh oh!

pshriwise commented Aug 17, 2021

Uh oh!

shimwell commented Aug 17, 2021

Uh oh!

pshriwise commented Nov 30, 2020 •

edited by gonuke

Loading

bam241 Jan 21, 2021 •

edited

Loading

pshriwise Jan 24, 2021 •

edited

Loading

pshriwise Jan 24, 2021 •

edited

Loading

pshriwise Jan 24, 2021 •

edited

Loading

pshriwise Jan 24, 2021 •

edited

Loading