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

Adding Cardinal model of a 7-Pin LFR demonstration case #360

Merged
merged 26 commits into from
Sep 26, 2024

Conversation

hapfang
Copy link
Contributor

@hapfang hapfang commented Feb 27, 2024

This PR is related to issue #359.

@moosebuild
Copy link

moosebuild commented Feb 27, 2024

Job VTB Documentation on 6d85ed0 wanted to post the following:

View the site here

This comment will be updated on new commits.

@hapfang hapfang changed the title Adding Cardinal model of a 7-Pin SFR demonstration case Adding Cardinal model of a 7-Pin LFR demonstration case Feb 27, 2024
@hapfang
Copy link
Contributor Author

hapfang commented Feb 29, 2024

Hi @GiudGiud @travismui, are we having significant degradation of SAM performance after recent update? The CI tests failed for 3 SAM models in the repository, and they are

The first SAM model timed out at step 57 while the tests tells it to run 100 steps.
The second one timed out at step 28.
The third one timed out at step 65.

Any help or suggestions are appreciated!.

@GiudGiud
Copy link
Collaborator

Did you invalidate and re run?

The performance of the computing cluster for testing also varies

@moosebuild
Copy link

Job Test SAM on 87f5940 : invalidated by @GiudGiud

invalidating to see if failure is reproducible

Copy link
Collaborator

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

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

quick first pass

lfr/7pin_cardinal_demo/7pin.usr Outdated Show resolved Hide resolved
lfr/7pin_cardinal_demo/HCmesh.i Show resolved Hide resolved
lfr/7pin_cardinal_demo/README Outdated Show resolved Hide resolved
lfr/7pin_cardinal_demo/tests Outdated Show resolved Hide resolved
lfr/7pin_cardinal_demo/tests Outdated Show resolved Hide resolved
lfr/7pin_cardinal_demo/tests Outdated Show resolved Hide resolved
lfr/7pin_cardinal_demo/tests Show resolved Hide resolved
@travismui
Copy link
Collaborator

Did you invalidate and re run?

The performance of the computing cluster for testing also varies

@GiudGiud Were the timeouts fixed by invalidating? I assumed it was just a slow job/node, our weekly VTB pipelines have been passing on our end.

@GiudGiud
Copy link
Collaborator

Yeah they were

Copy link
Collaborator

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

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

let s use the new module name.

We'll have to discuss the transfer setup.

@cticenhour
Copy link
Member

@hapfang Just in case you were not aware of this, I wanted to pass a long a GitHub tip. If you click on the "Files changed" tab, you can accept and then batch commit all suggested changes to a GitHub PR. This reduces the number of requests going to CIVET from a single PR and keeps the commit history tidy.

@hapfang
Copy link
Contributor Author

hapfang commented Mar 1, 2024

@hapfang Just in case you were not aware of this, I wanted to pass a long a GitHub tip. If you click on the "Files changed" tab, you can accept and then batch commit all suggested changes to a GitHub PR. This reduces the number of requests going to CIVET from a single PR and keeps the commit history tidy.

Good to know. I was not aware of it. Thanks for sharing.

@moosebuild
Copy link

Job Test SAM on e1e86b0 : invalidated by @hapfang

the same SAM models timed out, invalidate to reproduce the error

Copy link
Collaborator

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

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

let's add a renameBlockGenerator on the two meshes to have names on the subdomains on top of the ids

also the HC input cant be run on its own because the temperature seemingly blows up (thousands of K). I cant run the neutronics (no XS) but I imagine it's not the power density, it's the heat flux BCs that arent realisitically initialized?

@moosebuild
Copy link

Job Test griffin on f0a9725 : invalidated by @hapfang

redo the ci tests to confirm errors

@moosebuild
Copy link

Job Test blue_crab on f0a9725 : invalidated by @hapfang

@pak1sol
Copy link
Collaborator

pak1sol commented Jun 10, 2024

let's add a renameBlockGenerator on the two meshes to have names on the subdomains on top of the ids

also the HC input cant be run on its own because the temperature seemingly blows up (thousands of K). I cant run the neutronics (no XS) but I imagine it's not the power density, it's the heat flux BCs that arent realisitically initialized?

About temperature being blowing up, I could run the HC input without a problem. Since it uses the transient executioner while there's no time derivative and no changes in boundary condition, I just calculated three time steps and saw no changes in the solution after then. I see the initial condition for power density and boundary condition for the wall surface temperatures are set without a problem. Heat flux is the resulting quantity of the calculation, not the boundary condition. Can you elaborate more on your observation?
By the way, temperature goes up by thousands of K because of high power density and low heat conduction coefficient. Fuel temperature max should be around 2,000 K and the inner helium hole temperature should be even much higher.

Copy link
Collaborator

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

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

I'll need to think of something to explain why we turn off the transfersanity checks. It's clearly because we move data from nodes to centroids, and from centroids to nodes, and that's generally not a good idea.

Alternatively, let's use the machinery you all had me develop last year. So ProjectionAux to move the data from nodes to centroids, then transfer

lfr/7pin_cardinal_demo/HC.i Show resolved Hide resolved
lfr/7pin_cardinal_demo/tests Outdated Show resolved Hide resolved
@pak1sol
Copy link
Collaborator

pak1sol commented Jun 18, 2024

I'll need to think of something to explain why we turn off the transfersanity checks. It's clearly because we move data from nodes to centroids, and from centroids to nodes, and that's generally not a good idea.

Alternatively, let's use the machinery you all had me develop last year. So ProjectionAux to move the data from nodes to centroids, then transfer

So, such transfer occurs for heat flux from heat conduction to NekRS. The heat flux computed by heat conduction is an elemental variable and the receiving variable in NekRS is a nodal variable. I assume you suggested the opposite way. So you meant using ProjectionAux to move the heat flux variable in heat conduction from centroids to nodes and then transfer?

@GiudGiud
Copy link
Collaborator

So, such transfer occurs for heat flux from heat conduction to NekRS

Yes I think the heat flux is affected, and the temperature as well but in the opposite direction.

Ideally projecting before transferring will at least minimize the L2 norm of the difference between the fields, and will be reproducible in parallel unlike the NN approach

@pak1sol
Copy link
Collaborator

pak1sol commented Jun 18, 2024

So, such transfer occurs for heat flux from heat conduction to NekRS

Yes I think the heat flux is affected, and the temperature as well but in the opposite direction.

Ideally projecting before transferring will at least minimize the L2 norm of the difference between the fields, and will be reproducible in parallel unlike the NN approach

Right. Thanks. Let me add your description in the md file. By the way, temperature transfer from NekRS to heat conduction is nodal-to-nodal transfer. So only the heat flux has been the problem.

@pak1sol
Copy link
Collaborator

pak1sol commented Jun 18, 2024

@GiudGiud I've addressed your comments except for the test part changing RunApp to CSVDiff. I'll need to create gold files and update it soon.

@GiudGiud
Copy link
Collaborator

NONLINEAR should work.
I never remember this so I use Debug/show_execution_order=ALWAYS

HansolPark1 and others added 7 commits September 24, 2024 15:12
…pe to nodal type for transfer using ProjectionAux
…d being halved when being projected to nodal value
- add testing with more apps
- add headers to simulation inputs

Update lfr/7pin_cardinal_demo/runjob_sawtooth.sh

Update lfr/7pin_cardinal_demo/runjob_sawtooth.sh

Bring back delete fls file
Add gold files in the right folder
use Newton for HC solve for performance
@GiudGiud
Copy link
Collaborator

GiudGiud commented Sep 25, 2024

@hapfang do you know if this error means OOM?
https://civet.inl.gov/job/2463326/

or does it mean we need to compile nek ?

@GiudGiud
Copy link
Collaborator

looks like it was OOM , went a little further with 16 procs

@GiudGiud GiudGiud merged commit 354f7c6 into idaholab:devel Sep 26, 2024
13 checks passed
@GiudGiud
Copy link
Collaborator

thanks for the contribution!

@hapfang hapfang deleted the lfr2 branch September 26, 2024 04:27
@pak1sol
Copy link
Collaborator

pak1sol commented Sep 27, 2024

thanks for the contribution!

Thank you for your effort in getting this merged!!

@GiudGiud GiudGiud added the new_model Adding a new model to the VTB label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new_model Adding a new model to the VTB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants