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

cad_geometry_example #29

Closed
wants to merge 25 commits into from
Closed

Conversation

meltawila
Copy link
Collaborator

@meltawila meltawila commented Apr 24, 2024

This PR is to add CAD-based Geometry Workflow example for Multiphysics Fusion Problems Using OpenMC and MOOSE. Work and results taken from M. Eltawila, A. J. Novak, P.C. A. Simon, G. Giudicelli, D. Gaston, “Investigating CAD-based Geometry Workflows for Multiphysics Fusion Problems Using OpenMC and MOOSE,” Submitted to Pacific Basin Nuclear Conference (PBNC) (2024)

(Ref. #30)

@simopier simopier self-assigned this Apr 24, 2024
Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

First of all, thank you for contributing this example to FENIX! I'm thrilled to see it added!
A few things to keep in mind:

  1. You are currently getting the error:
ERROR: Your patch does not contain a valid ticket reference! (i.e. #1234)
Merge branch 'cardinal_example' of github.com:meltawila/fenix into test
cad_geometry_example

This is because you need an issue number in one of your commits (and it's also best to include it in the description at the top of your PR). I just created an issue for you and here is what you want to provide: (Ref. #30). In the future, make sure to create an issue before creating the PR (or even before creating your first commit). You can find guidance for that here: https://fenix-dev.hpc.inl.gov/site/sqa/contributing.html

  1. You are currently getting the error:
ERROR: The following files contain trailing whitespace after applying your patch:
	doc/content/examples/cad_geometry_model/input_files/model.py

Run the "delete_trailing_whitespace.sh" script in your $MOOSE_DIR/scripts directory.

Follow this guidance, or, in VSCode, you can go to Code->settings->settings
then type whitespace, and then you can select a couple of options, including Editor:trim auto whitespace and File:trim trailing whitespace, which should prevent you from running into this issue in the future.

  1. You are currently getting the error:
ERROR: The following files contain windows line endings:
	doc/content/examples/cad_geometry_model/input_files/model.py

Make sure to remove those.

  1. Just like in a paper, make sure to reference the figures in the text in the documentation.
  2. The current location of the input files is not appropriate, but I'll create a PR to prepare things a little better in FENIX and will tell you where to move them. You will also need a test file to run this case and test it, as well as some gold files that will contain the test results, and then ensure that the results remain consistent as FENIX gets further developed.
  3. To address these comments, you can go to 'Files Changes' at the top of this page and add suggestions to batch when you agree with them. This will create a commit with these changes.
    Then, pull this PR locally and make further edits
    To pull a PR locally, do:
git checkout <branch_name> 
git branch --set-upstream-to=origin/<branch_name> 
git fetch origin <branch_name> 
git status # to check that fetch pulled new commits and that tracking is showing the commit difference 
git pull

Let me know if you have any questions.
Thanks!

doc/content/examples/cad_geometry_model/cad_model.md Outdated Show resolved Hide resolved
doc/content/examples/cad_geometry_model/cad_model.md Outdated Show resolved Hide resolved
doc/content/examples/cad_geometry_model/cad_model.md Outdated Show resolved Hide resolved
doc/content/examples/cad_geometry_model/cad_model.md Outdated Show resolved Hide resolved
doc/content/examples/cad_geometry_model/cad_model.md Outdated Show resolved Hide resolved
doc/content/examples/cad_geometry_model/cad_model.md Outdated Show resolved Hide resolved
Co-authored-by: Pierre-Clement Simon <[email protected]>
@simopier
Copy link
Collaborator

@meltawila: #33 is merged, so you should have everything you need to reorganize this PR.
Let me know if you have any questions and need more clarity about the instructions I provided above.

meltawila and others added 3 commits May 20, 2024 22:25
Co-authored-by: Pierre-Clement Simon <[email protected]>
Merge devel into my branch to move the files into the correct path
Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

The tests are failing for the reasons described in my comment above. Use the guidance I provided to fix these.

@moosebuild
Copy link
Contributor

moosebuild commented May 23, 2024

Job Documentation on 09a3c46 wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented May 23, 2024

Job Coverage on 09a3c46 wanted to post the following:

Coverage

Coverage did not change

Full coverage report

This comment will be updated on new commits.

Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

Thank you @meltawila!

I provided some changes to fix the tests that are currently failing.

The main changes that you still have to do are:

  • Create a new folder under test/tests' titled cad_geometry_exampleand move the content of the currentinput_files` folder you created there. The input files and mesh should be in the test folder, not the documentation folder.
  • Once you have created the cad_geometry_example folder and moved the input files and mesh there, then you'll need to create a tests file like you can find here that automatically run the input file as part of a test for FENIX.
  • Then, you will need to create - within test/tests/cad_geometry_example - a gold folder that contains the outputs created by your tests. This file will be used in the future to ensure that this example case still reproduces the same output as FENIX evolves (if one day it doesn't, then it means something is broken, or the gold file should be updated). This is a crucial part of our Software Quality Assurance.
  • I would also like to see, in the documentation, a section dedicated to listing the instructions to run the example case that you are creating there.

There are also a few comments from previous reviews that were not fully addressed (see above).

Let me know if you have any questions.

@simopier
Copy link
Collaborator

simopier commented May 26, 2024

@meltawila
In the future, make sure to add descriptive commit messages to document what you do rather than just ..

Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

Starting to look really good! Thank you @meltawila!
Make sure to add the tests file and a gold folder as described in previous reviews.

Also, tmesh_1.e is missing from the PR.

@moosebuild
Copy link
Contributor

Job Precheck on 09a3c46 wanted to post the following:

Warning: This PR changes repo size by 12.59 MiB.

Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

Some minor comments. I'll follow your instructions locally and let you know if there's anything else that should be added to the documentation or these input file.

type = Exodiff
input = 'solid.i'
exodiff = solid_out.e
requirement = "The system shall be able to solve for the temperature distribution."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
requirement = "The system shall be able to solve for the temperature distribution."
requirement = "The system shall be able to run a coupled neutronics and heat conduction simulation and solve for the temperature distribution."

type = Exodiff
input = 'solid.i'
exodiff = solid_out_openmc0.e
requirement = "The system shall be able to solve for the heat source and tritium production."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
requirement = "The system shall be able to solve for the heat source and tritium production."
requirement = "The system shall be able to run a coupled neutronics and heat conduction simulation and solve for the heat source and tritium production."

[openmc_diff]
type = Exodiff
input = 'solid.i'
exodiff = solid_out_openmc0.e
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
exodiff = solid_out_openmc0.e
exodiff = solid_out_openmc0.e
should_execute = False # this test relies on the output files from solid_diff, so it shouldn't be run twice
prereq = solid_diff

Copy link
Collaborator

Choose a reason for hiding this comment

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

This prevents the same test from being run twice, it simply leverages the results from the previous test.

Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

The test are actually skipped with the message:

SKIP test:cad_geometry_example.openmc_diff [OpenMCCellAverageProblem not found in executable]
SKIP test:cad_geometry_example.solid_diff [OpenMCCellAverageProblem not found in executable]

You should add to the PR (or a separate one) the changes that need to happen in FENIX to be able to run Cardinal/OpenMC.

@simopier simopier closed this Jun 25, 2024
@simopier
Copy link
Collaborator

@meltawila you might need to follow the instructions described at the bottom of #43.

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