Add function to expose find_volume in DAGMC

[gonuke]

Description

Allow physics codes to directly call find_volume() that has been implemented in MOAB

Motivation and Context

In some circumstances, there is a need to identify the current volume based on the current position of a particle. Some codes implement their own search process and only require a check for point inclusion (point_in_volume()). This function supersedes the search process built in to the physics code and takes advantage of OBB acceleration to perform the search. This change exposes it in the DagMC interface.

Changes

This is a feature addition to connect an existing feature of MOAB to the DagMC interface.

[gonuke]

This requires an update to MOAB 5.2.x

[shimwell]

This requires an update to MOAB 5.2.x

Wondering if the MOAB 5.3 help with this volume exposing

[pshriwise]

The code changes here look good to me! I'm thinking we should impose a minimum version requirement for MOAB in CMakeLists.txt. From the MOAB history, it looks like that version would be 5.2.0.

[gonuke]

The code changes here look good to me! I'm thinking we should impose a minimum version requirement for MOAB in CMakeLists.txt. From the MOAB history, it looks like that version would be 5.2.0.

Now I have to figure out how...?

[pshriwise]

The code changes here look good to me! I'm thinking we should impose a minimum version requirement for MOAB in CMakeLists.txt. From the MOAB history, it looks like that version would be 5.2.0.

Now I have to figure out how...?

Should just be a version check on MOAB_VERSION after the find_package call, but sadly I don't see that variable being set in the MOAB CMake config files that get installed with the package. 😞 It's in the pkconfig files, but not the CMake versions unfortunately.

[gonuke]

I just submitted a PR in MOAB for this (https://bitbucket.org/fathomteam/moab/pull-requests/571), but given that they just did a microrelease, not sure how long it will take to emerge.

If you think we can go ahead without this, I've submitted #788 to track this for the future.

[pshriwise]

I just submitted a PR in MOAB for this (https://bitbucket.org/fathomteam/moab/pull-requests/571), but given that they just did a microrelease, not sure how long it will take to emerge.

Sounds good. Thanks for doing that!

If you think we can go ahead without this, I've submitted #788 to track this for the future.

Also sounds good. How would you feel about waiting until after the upcoming release then? I think it's fairly likely that someone will try to build the latest version of DAGMC with an older version of MOAB that doesn't contain this function.

[gonuke]

It turns out that the version info is already available as a preprocessor variable, so I added #if guards based on the version major and minor numbers. This may be a better solution(?) that requiring a newer version since some folks may not want this feature and still want to use an older MOAB.

[pshriwise]

My concern with relying on #ifdef's here is that it may obfuscate the cause of a missing find_volume function in a downstream application should someone mistakenly build with an older version of MOAB. If I understand the plan correctly, a compile error would appear when trying to build the downstream application even though DAGMC will have compiled without errors.

I think it would be clearer to require the version of MOAB that has this function during configuration or at least causes the DAGMC build to fail. To do this without relying on the MOAB version in the CMAKE config file I can think of a couple of options:

• To fail during configuration we could include a simple test program that will return success or failure based on the MOAB version. DAGMC's CMake can build that program with execute_process(). We'd then catch that return value in CMake and display an appropriate error message. (messy, but fails during config)
• This one might be more helpful, but less clear to newcomers. Add a find_volume test that will fail to compile if that function isn't present in MOAB. We might be able to include a valuable error message here as well? (new test! 😁 , fails during build rather than compilation 😞 )

[gonuke]

So maybe this should be in a minor release (3.3.0) rather than a micro release (3.2.1) since it's an API change, albeit an additive one

[pshriwise]

I could've sworn we'd merged this in, but I was looking yesterday and sure enough, we have not! Shall we revive this one @gonuke?

[gonuke]

I'll try to port the changes over for this. I thought we merged it too!!

[pshriwise]

Based on the code changes, I'm happy to merge this ASAP. Should we do another CI run before that though?

[gonuke]

rebased, tested & ready to merge @pshriwise