Skip to content

Conversation

@GiudGiud
Copy link
Contributor

@GiudGiud GiudGiud commented Nov 6, 2023

Open for comments.
Once the dependent PRs are in, this could actually be merged
refs #25642
see original PR #25704

I dont think the syntax has been decided yet, so I m open to a CCB meeting on this

Currently the Physics base class only implements:

  • some basic parameter checking
  • an insertion point for relationship managers at the equation level (could be removed for now, will come in for NS)
  • an insertion point for restarting from Exodus (could be removed for now, will come in for NS)

@GiudGiud GiudGiud self-assigned this Nov 6, 2023
@moosebuild
Copy link
Contributor

moosebuild commented Nov 6, 2023

Job Documentation on bd7ff03 wanted to post the following:

View the site here

This comment will be updated on new commits.

@GiudGiud GiudGiud force-pushed the PR_hc_physics branch 4 times, most recently from 9a69fca to 9a218a6 Compare November 15, 2023 21:40
@moosebuild
Copy link
Contributor

moosebuild commented Nov 16, 2023

Job Coverage on bd7ff03 wanted to post the following:

Framework coverage

796cb8 #25977 bd7ff0
Total Total +/- New
Rate 85.24% 85.20% -0.04% 74.95%
Hits 99138 99509 +371 377
Misses 17166 17283 +117 126

Diff coverage report

Full coverage report

Modules coverage

Heat transfer

796cb8 #25977 bd7ff0
Total Total +/- New
Rate 88.28% 88.16% -0.11% 83.49%
Hits 3938 4029 +91 91
Misses 523 541 +18 18

Diff coverage report

Full coverage report

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 74.95% is less than the suggested 90.0%
  • heat_transfer new line coverage rate 83.49% is less than the suggested 90.0%

This comment will be updated on new commits.

@GiudGiud GiudGiud force-pushed the PR_hc_physics branch 3 times, most recently from ec9b742 to 89d27ff Compare November 28, 2023 00:00
@GiudGiud
Copy link
Contributor Author

GiudGiud commented Nov 28, 2023

Should probably move this forward. It doesnt have to be merged right right now, we could also wait until I dig deep into Component as Actions and assess if this is going to change the deal for Physics. I dont expect it to. But work on syntax for Components as Actions could enable interesting syntax for Physics.

@GiudGiud GiudGiud marked this pull request as ready for review November 29, 2023 22:53
@GiudGiud GiudGiud requested a review from lindsayad as a code owner November 29, 2023 22:53
@GiudGiud GiudGiud force-pushed the PR_hc_physics branch 2 times, most recently from 248ffe2 to 87ec16c Compare November 30, 2023 13:36
Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

want to add a framework test?

@GiudGiud
Copy link
Contributor Author

GiudGiud commented Dec 4, 2023

need a framework class then? Thoughts on which physics? A test object?

@lindsayad
Copy link
Member

The thing that came to my mind is DiffusionPhysics or PoissonPhysics. @nmnobre just recently created a coupled_elestrostatics.i framework level test in his Raviart-Thomas PR.... It could be a good demonstration of Physics to make both CFEM and Raviart-Thomas implementations of the Poisson equation.... Although I can't remember if you wanted Physics to be useful for the purpose of allowing different algebraic discretizations of the same physics. Also you probably don't want to balloon this PR too much, so maybe just a CFEM implementation of Poisson would be good to start, and then we could add RT later.

I thought we had an issue where we had a pretty large community (I thought it involved @dschwen at least) discussion about the Physics class but maybe i've lost my marbles. #25642 doesn't seem like it.

@GiudGiud
Copy link
Contributor Author

GiudGiud commented Dec 5, 2023

I'm happy to implement that.

@GiudGiud
Copy link
Contributor Author

GiudGiud commented Dec 5, 2023

A lot of the discussion is on the original Physics PR #25704

wanted Physics to be useful for the purpose of allowing different algebraic discretizations of the same physics

I do want that. I dont think it s feasible to do a [Discretization] block with our current ways of parsing the input file without being very convoluted. So I think when we instantiate a Physics object in the input file, it should have the discretization as well

@lindsayad
Copy link
Member

lindsayad commented Dec 5, 2023

A lot of the discussion is on the original Physics PR #25704

Ah there it is, thanks

@GiudGiud GiudGiud requested a review from roystgnr as a code owner December 8, 2023 01:05
@GiudGiud GiudGiud force-pushed the PR_hc_physics branch 2 times, most recently from b84486e to 3c57e8f Compare December 8, 2023 15:00
@GiudGiud
Copy link
Contributor Author

Let me know when this is ready for a thorough review. I'll have a lot of requests for documentation, but I'd probably just have us fix anything wrong and then I can do a follow-up PR to improve the documentation to my satisfaction, and you can review that.

Alex has been thoroughly reviewing this. I dont think there is anything left wrong! He did catch a lot of wrong things

@joshuahansel
Copy link
Contributor

I dont think there is anything left wrong! He did catch a lot of wrong things

Oh don't worry, there's wrong stuff still. But just doc/typos so far 😄

@GiudGiud GiudGiud force-pushed the PR_hc_physics branch 3 times, most recently from ecbbbcc to 41bd218 Compare January 25, 2024 06:22
@GiudGiud GiudGiud force-pushed the PR_hc_physics branch 2 times, most recently from 46811a1 to 859189e Compare January 25, 2024 16:39
@idaholab idaholab deleted a comment from moosebuild Jan 25, 2024
- fix documentation

Apply suggestions from code review

Co-authored-by: joshuahansel <[email protected]>
@joshuahansel
Copy link
Contributor

You should probably make a newsletter entry

@GiudGiud
Copy link
Contributor Author

GiudGiud commented Jan 25, 2024

I ll make one for all of my January work soon

@GiudGiud GiudGiud merged commit 55f701f into idaholab:next Jan 30, 2024
@GiudGiud GiudGiud deleted the PR_hc_physics branch January 30, 2024 00:47
@GiudGiud
Copy link
Contributor Author

noice thanks a lot for the reviews. Merging so it makes the newsletters

now for the tough part. NS, TM and THM

nmnobre added a commit to nmnobre/moose that referenced this pull request Nov 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants