Ensure implicit complement is placed at the back of the DAGMC index space

pshriwise commented 6 months ago

Please follow these guidelines when making a Pull Request. This template was adapted from here and here.

Description

This PR enforces placement of the implicit complement handle at the back of the surfaces and volumes vectors (contained in the DagMC::entHandles data member). It also changes the EntityHandle -> DAGMC index mapping structure (DagMC::entIndices) s.t. DAGMC indexes are independent of the EntityHandle ordering produced by MOAB. I decided on std::unordered_map for a lookup complexity O(1). The downside here is that the container use more memory, but this should be a small percentage of the overall memory use in DAGMC as I understand it. If others have approaches for maintaining the std::vector<int> option I'm all ears, but I have a feeling it would be more trouble than it's worth.

I've confirmed that this results in the same eigenvalue as MOAB 5.3.0 for the MSRE model mentioned in #934 with OpenMC.

Motivation and Context

The motivation for this is described in #934.

Changes

Manual placement of the implicit complement handle at the back of the entHandles container.
Structural change in the container type used for DAGMC indices.

Behavior

In some cases, the implicit complement handle may not be placed at the back of the DAGMC index space and could be checked in point containment queries before other explicit volumes in the model.

gonuke commented 6 months ago

This housekeeping failure has dogged me before... there is some inconsistency that I haven't bothered to get to the bottom of, and it requires me to manually override the style guide for this line.

pshriwise commented 6 months ago

Looks good to me - should we add a test for this? I guess we'd have to contrive a set of volumes where the implicit complement was in the wrong place?

Yeah, we'd ideally test this. I think we could do it by using the DacMC constructor that takes a MOAB instance. I think we could artificially occupy a bunch of MeshSet handles before passing it to DAGMC to ensure that loading a DAGMC file triggers this situation.

ahnaf-tahmid-chowdhury commented 6 months ago

You mean we will use a sample model (DAGMC file) in DacMC constructor and then we will check the output with predefined results?

gonuke commented 6 months ago

We would probably not use a file. Probably manually create a minimal DAGMC model in memory for testing.

ahnaf-tahmid-chowdhury commented 6 months ago

Thanks for the clarification! Creating a minimal DAGMC model in memory for testing sounds like a good approach to minimize source code size.

I'm always happy to contribute by making a PR and generating the test file. However, I'm unsure about the process for creating tests for DAGMC. Could you please provide a sample or some guidance to help me get started?

gonuke commented 6 months ago

Proposed minimal model:

3 volumes created in order 1, 2, 3
populate set 1 with surfaces/facets that represent a central cube
populate set 3 with surfaces/facets that represent an outer graveyard prismatic shell
populate set 2 as the implicit complement built from the surfaces of sets 1 & 2

Load all of that into a MOAB entity as if it was load_file and then run the code to test this PR's addition.

gonuke commented 6 months ago

Alternative:

load a file with an existing implicit complement (IC)
create a new volume set (assumed to be added AFTER the IC)
move all the topological relationships from some other volume set (the first one?) to this new one
delete the original volume set

Run these methods to move the IC and test

pshriwise commented 5 months ago

Proposed minimal model:

3 volumes created in order 1, 2, 3

populate set 1 with surfaces/facets that represent a central cube

populate set 3 with surfaces/facets that represent an outer graveyard prismatic shell

populate set 2 as the implicit complement built from the surfaces of sets 1 & 2

Load all of that into a MOAB entity as if it was load_file and then run the code to test this PR's addition.

I ended up taking a slightly different approach here. To trigger this condition we need to load a file with at least DEFAULT_MESHSET_SEQUENCE_SIZE (currently set to 16 * 1024) sets. I've loaded a test DAGMC file, added a bunch of sets to it (we technically only need 16,384, but I've used one million to be safe), we then write a temporary file, read it back in, and setup the DAGMC implicit complement and indices. All of the additional sets should be ignored, but the file read should still trigger this condition.

To convince myself that this is true, I created a branch that adds this test on top of develop to ensure that it fails with the implicit complement placed at the beginning of the DAGMC index space.

https://github.com/pshriwise/DAGMC/actions/runs/7748743790/job/21131962325#step:5:100

But with the latest CI run, that same test now passes:

https://github.com/svalinn/DAGMC/actions/runs/7748757514/job/21132001898?pr=935

I rather like this test as it demonstrates that the behavior is caused by the number of meshsets in the file, even if they have no bearing on the definition of a DAGMC model.

svalinn / DAGMC