CEMeNT-PSAAP / MCDC

MC/DC: Monte Carlo Dynamic Code
https://mcdc.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
20 stars 20 forks source link

Improving CSG and input cards #198

Closed ilhamv closed 2 months ago

ilhamv commented 3 months ago

The PR extends the CSG modeling features by supporting not only intersection but also union and complement operators. To achieve that, "region class" is introduced.

Major changes:

ilhamv commented 3 months ago
ilhamv commented 3 months ago

Hi @clemekay . I made some modifications to the UQ items so that it is working with the new input card structure. Can you please check if they are OK? (It pass the VADE regression test already, though)

ilhamv commented 3 months ago

Hi @spasmann . Some of the iQMC regression tests give very unusual results...

image

What do you think about that?

ilhamv commented 3 months ago

@spasmann , I think the issue has something to do with running iQMC with reflecting BC. Lots of particle loss incidents in iqmc_cooper2 and iqmc_reed in the regression test.

Please let me know what you think. Thanks!

ilhamv commented 2 months ago

We will make a followup PR on revising the examples.

jpmorgan98 commented 2 months ago

A few questions:

  1. Do we want to merge #204 before this?
  2. Did you check to make sure this new syntax is still compatible with GPU compilation?
  3. I believe this will change how are input decks are managed which will mean we should go to 0.11.0 for our next version
  4. Do we need to update the documentation cite?
ilhamv commented 2 months ago

A few questions:

  1. Do we want to merge iQMC Refactor #204 before this?
  2. Did you check to make sure this new syntax is still compatible with GPU compilation?
  3. I believe this will change how are input decks are managed which will mean we should go to 0.11.0 for our next version
  4. Do we need to update the documentation cite?
  1. Looks like it happened already. The conflict was easily resolved though. 👍🏽
  2. I haven't. Can you please check that one, @jpmorgan98 ?
  3. Yes.
  4. Yes. But I think this can be a separate PR as it is happening in dev not in main.
jpmorgan98 commented 2 months ago

Ya I will run the GPU checks!

jpmorgan98 commented 2 months ago

GPU compilation fails for all problems. The only error that is printed to screen is,

Aborted (core dumped)

@braxtoncuneo could this be because some of some changing data layout when implementing new tracking method? Also somthing to note is that the global int like BC and FILL_LATTICE: are never declared and/or moved to the GPU. I am guessing they are supposed to be stored in the global state but that feels like the most significant diffrence in this PR.

As discussed in our meeting we are going to approve the PR into dev with the condition that this must be resolved before next version release. I am setting up an issue to track this. When that is done I will merge this PR

braxtoncuneo commented 2 months ago

GPU compilation fails for all problems. The only error that is printed to screen is,

Aborted (core dumped)

@braxtoncuneo could this be because some of some changing data layout when implementing new tracking method? Also somthing to note is that the global int like BC and FILL_LATTICE: are never declared and/or moved to the GPU. I am guessing they are supposed to be stored in the global state but that feels like the most significant diffrence in this PR.

As discussed in our meeting we are going to approve the PR into dev with the condition that this must be resolved before next version release. I am setting up an issue to track this. When that is done I will merge this PR

I will check this out.

Nothing seems immediately off about _type.py, but memcheck should hopefully provide insight.

The values of globally declared variables are "baked" into JIT'd functions. This effectively makes them constants that happen to be defined by whatever value the global variable is at the time of compilation. Unless MCDC is changing these values and relying upon those changes to take effect in compiled code (which I doubt is the case), this should be fine for the GPU code. Since the values are baked into the compiled GPU functions anyway, nothing would need to be moved around.