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

Remove NSFV Action and NSFV component, use Physics #27951

Open
wants to merge 8 commits into
base: next
Choose a base branch
from

Conversation

GiudGiud
Copy link
Contributor

refs #25642

@GiudGiud GiudGiud self-assigned this Jun 20, 2024
@moosebuild
Copy link
Contributor

moosebuild commented Jun 20, 2024

Job Documentation on 805ae63 wanted to post the following:

View the site here

This comment will be updated on new commits.

@GiudGiud GiudGiud marked this pull request as ready for review June 20, 2024 17:49
@GiudGiud GiudGiud force-pushed the PR_remove_ns_action branch 3 times, most recently from c1e4cfc to bd99916 Compare June 23, 2024 05:06
@lindsayad
Copy link
Member

Are the TH area users ready for this?

@GiudGiud
Copy link
Contributor Author

@licharlot @tanoret

@GiudGiud
Copy link
Contributor Author

Note that the old syntax remains completely functional. It's just the backend that changes (towards something that uses mostly the same code, just organized differently).
You do get a deprecation message.

@GiudGiud
Copy link
Contributor Author

GiudGiud commented Jun 24, 2024

Public app failure is fake
EDIT:
non-unity was not, but fixed now

@lindsayad
Copy link
Member

Note that the old syntax remains completely functional. It's just the backend that changes (towards something that uses mostly the same code, just organized differently).
You do get a deprecation message.

Oh nice 👍

@moosebuild
Copy link
Contributor

moosebuild commented Jun 25, 2024

Job Coverage on 805ae63 wanted to post the following:

Framework coverage

Coverage did not change

Modules coverage

Navier stokes

62dc98 #27951 805ae6
Total Total +/- New
Rate 84.01% 84.58% +0.57% 99.28%
Hits 17336 16196 -1140 276
Misses 3300 2952 -348 2

Diff coverage report

Full coverage report

Thermal hydraulics

62dc98 #27951 805ae6
Total Total +/- New
Rate 89.14% 88.86% -0.28% -
Hits 14191 14111 -80 0
Misses 1729 1769 +40 0

Diff coverage report

Full coverage report

Full coverage reports

Reports

This comment will be updated on new commits.

Copy link
Contributor

@joshuahansel joshuahansel left a comment

Choose a reason for hiding this comment

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

I want to discuss

Had to regold because using a single physics for both components and the two components were using different advection methods. This is not one of the block restricted parameters.
The other solution would have been to use two physics. This would necessitate different variable names at the moment. There were some work on merging physics together
It is possible, it's also quite a bit of code to achieve something that so far is not really needed

In particular the part about needing different variables if needing more than one physics. I just want to understand any limitations before tighter integration between components and physics. But the change to this test is probably fine.

expect_err = 'rho: The density in the boussinesq term is not constant!'
requirement = 'The system should throw an error if the density is not a constant functor in case of Boussinesq treatment.'
valgrind = 'none'
issues = '#19742'
# New physics cannot support this because we want properties forwarded from fluid properties to work
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand. So the Physics version is not going to take density as a functor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the test for detecting whether a functor is constant doesn't work reliably. It just calls isConstant but many constants are not created in ConstantFunctors

Copy link
Contributor

Choose a reason for hiding this comment

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

So how does Physics affect this? And does this test make sense to exist? Do we just need to improve isConstant somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we just need to improve isConstant somehow?

I dont see an easy way. Save for maybe evaluating it all over the mesh to check.

So how does Physics affect this?

I dont want to limit incompressible flow to not using the FP properties and the GFFP. I ll give it another try to see if I can re-enable this.

@GiudGiud
Copy link
Contributor Author

GiudGiud commented Jun 25, 2024

Unless we start changing the block restrictions of created objects, I don't see how we could not use different variable names.

The physics create block restricted variables and UOs. If we have two physics, we would need them to modify the block restriction.

My preference is a single physics, with block restricted parameters on the components that fill the maps I have been creating to hold the parameters

@joshuahansel
Copy link
Contributor

Ok, I'm good with these changes, so just checking for any dissent by @licharlot or @tanoret before approval.

More verbose block restriction error
refs idaholab#25642
Remove FlowComponentNS: covered by FileMeshPhysicsComponent
refs idaholab#25642
Had to regold because using a single physics for both components and the two components were using different advection methods. This is not one of the block restricted parameters.
The other solution would have been to use two physics. This would necessitate different variable names at the moment. There were some work on merging physics together
It is possible, it's also quite a bit of code to achieve something that so far is not really needed
refs idaholab#25642
The new default is true, as we have to assume the user does not want an extra multiplication by
porosity, as most PGH models do not require it
refs idaholab#25642
@moosebuild
Copy link
Contributor

Job Coverage on 805ae63 wanted to post the following:

The following coverage requirement(s) failed:

  • thermal_hydraulics coverage rate 88.86% is less than the required 89.0%

@GiudGiud
Copy link
Contributor Author

let's go?

@licharlot
Copy link
Contributor

Looks good to me. Thanks for adding the transition tutorial.

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.

None yet

5 participants