Skip to content
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

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

Merged
merged 10 commits into from
Feb 4, 2024

Conversation

pshriwise
Copy link
Member

@pshriwise pshriwise commented Jan 11, 2024

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
Copy link
Member

gonuke commented Jan 11, 2024

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.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

src/dagmc/DagMC.hpp Show resolved Hide resolved
@pshriwise
Copy link
Member Author

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
Copy link
Member

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

@gonuke
Copy link
Member

gonuke commented Jan 15, 2024

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

@ahnaf-tahmid-chowdhury
Copy link
Member

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
Copy link
Member

gonuke commented Jan 25, 2024

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
Copy link
Member

gonuke commented Jan 25, 2024

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
Copy link
Member Author

pshriwise commented Feb 1, 2024

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

image

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

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

image

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.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this @pshriwise

@gonuke gonuke merged commit 1153d75 into svalinn:develop Feb 4, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants