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

Fix for UWUW Macro Conflict #3150

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

ahnaf-tahmid-chowdhury
Copy link

Description:

This PR resolves the following issues in the OpenMC DAGMC interface:

  1. Macro Conflict Fix:

    • The UWUW macro was causing conflicts during compilation as it was treated as a numeric constant.
    • To resolve this, the macro UWUW has been replaced with UWUW_HPP to avoid naming conflicts with numeric constants.

    Changes:

    • In CMakeLists.txt, the line:

      target_compile_definitions(libopenmc PRIVATE UWUW)

      has been updated to:

      target_compile_definitions(libopenmc PRIVATE UWUW_HPP)
    • All relevant files that referenced UWUW have been updated accordingly.

  2. Const Qualifier Removal:

    • The const qualifier was removed from the function uwuw_assign_material in both the declaration (in dagmc.h) and the definition (in dagmc.cpp).
    • This allows for modifying class member variables within the function, which is necessary for proper material assignment in the DAGMC interface.

@shimwell
Copy link
Member

Looks like a file is not being found

/home/runner/work/openmc/openmc/src/dagmc.cpp:15:10: fatal error: uwuw.hpp: No such file or directory
   15 | #include "uwuw.hpp"
      |          ^~~~~~~~~~
compilation terminated.

Copy link
Contributor

@pshriwise pshriwise 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 this update @ahnaf-tahmid-chowdhury! My only request is that we change the new precompiler value from DAGMC_UWUW to OPENMC_UWUW, using the OPENMC_ prefix as something of a namespace for these precompiler values. We tend to do this with the CAPI functions as well.

@ahnaf-tahmid-chowdhury
Copy link
Author

However, some tests are failing with the following error:

ERROR: Material with name/ID 'fuel' not found for volume (cell) 1

It seems that in the recent OpenMC version, all materials must include all the same properties, can't keep anything empty. We need to update the dagmc.h5m file, but I’m not sure how to do that.

@shimwell
Copy link
Member

shimwell commented Oct 4, 2024

I might be wrong here but that error looks like the openmc materials provided don't include a material with the name 'fuel'.

I don't know why this passed previously but I guess checking the model.materials contain a material with the name fuel would help check and then perhaps the material name can be changed to fuel if it does not exist.

@ahnaf-tahmid-chowdhury
Copy link
Author

I don't know why this passed previously but I guess checking the model.materials contain a material with the name fuel would help check and then perhaps the material name can be changed to fuel if it does not exist.

Unfortunately, it was turned off when this was created. I have recently turned it on and found that the related tests are now failing, as it was not updated over time. We should consider turning on all features in our CI to ensure everything is working properly.

@pshriwise
Copy link
Contributor

I don't know why this passed previously but I guess checking the model.materials contain a material with the name fuel would help check and then perhaps the material name can be changed to fuel if it does not exist.

Unfortunately, it was turned off when this was created. I have recently turned it on and found that the related tests are now failing, as it was not updated over time. We should consider turning on all features in our CI to ensure everything is working properly.

I looked into this. I think the .h5m file for this test is fine. The problem originates in #2965 when the read_uwuw_materials() was moved from the initialize method to the uwuw_assign_material method. This was not caught in review or in CI because in CI UWUW was never enabled, as you pointed out @ahnaf-tahmid-chowdhury. Now that UWUW enabled the failure is known, I've pushed a change to this PR moving that call back into the initialize method. The test passes locally for me now. I'll keep an eye on CI here.

@pshriwise
Copy link
Contributor

Looks like that test is passing now. I'll take a look at the changes again shortly.

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

A couple of tweaks for default behavior and one correction to the config file. Ideally we'd our OpenMCCMake.config with some small application that builds on top of the OpenMC installation. @ahnaf-tahmid-chowdhury would you mind making an issue for that task?

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
@@ -34,7 +34,7 @@ option(OPENMC_USE_LIBMESH "Enable support for libMesh unstructured mesh tall
option(OPENMC_USE_MPI "Enable MPI" OFF)
option(OPENMC_USE_MCPL "Enable MCPL" OFF)
option(OPENMC_USE_NCRYSTAL "Enable support for NCrystal scattering" OFF)
option(OPENMC_USE_UWUW "Enable UWUW" OFF)
option(OPENMC_USE_UWUW "Enable UWUW" ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the comment below, let's make the default for this option OFF.

cmake/OpenMCConfig.cmake.in Outdated Show resolved Hide resolved
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