Enable multiple objects, postprocs, transfers for FV variables #16115

GiudGiud · 2020-11-08T02:42:46Z

Enable the following pp, user objects and transfers to work with FV variables:

ElementIntegralArrayVariablePostProcessor
ElementVariablePostProcessor
InterfaceIntegralVariableValuePostProcessor
Side IntegralVarablePostProcessor
ElementIntegralVariableUserObject
SideIntegralVariableUserObject
PointSampleBase derived vector pp
MultiAppVariableValueSampleTransfer
MultiAppVariableValueSamplePostProcessorTransfer

For the user objects, they are bases for other user objects. Should I add tests with fv for all of them or just one?
For the fix in the phase field module, should I add a test? Not sure FV is used for phase field modeling.

…6099

…daholab#16099

…aholab#16099

…est for ElementVariableValuePP. idaholab#16099

moosebuild · 2020-11-09T04:53:44Z

Job Precheck on 6e81f25 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/16115/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 56c8c77449cf3c6e444c1abeb680597750da0cb2

rwcarlsen · 2020-11-09T20:26:47Z

Regarding test questions - just make sure that ideally ~every modified chunk of code gets hit by at least one test - i.e. if you undo any single (substantive) change in this PR - you would want at least one test to break.

GiudGiud · 2020-11-09T20:50:09Z

Ok one test per base object should be able to do this then. I will add them soon. Thanks for the guidance

rwcarlsen · 2020-11-10T23:36:56Z

Alright - are you ready for a review on this?

GiudGiud · 2020-11-11T00:00:51Z

Sure!
I'll take care of postprocessors/interface_value.interface_integral_variable_value_postprocessor_fv_test rn but dont let it stop you
it's failing because I started from a test using stateful properties and they arent supported in FV

EDIT Actually I used those in 2 different tests and the other did fine. I wonder what's special about this one.
I don't think it's worth getting rid of stateful materials here, the tests are pretty well designed, we just have to wait for FV stateful material support to enable them.
I raised an issue and added a skip on those two tests.

If the workaround is not satisfactory I think I would just fix stateful materials with FV.

moosebuild · 2020-11-11T00:40:59Z

Job Documentation on 21e9e5a wanted to post the following:

View the site here

This comment will be updated on new commits.

…daholab#16149

rwcarlsen

Looks good. Just a couple minor things.

rwcarlsen · 2020-11-12T15:57:07Z

test/tests/postprocessors/element_integral_var_pps/tests

@@ -18,7 +18,15 @@
      input = 'pps_old_value.i'
      exodiff = 'pps_old_value_out.e'

-      detail = 'at the end of each time step.'
+      detail = 'at the end of each time step, for both FE and'


'... and FV.'

ok moved the and to the next detail.
Not sure where these go yet, I just copied what was being done in the file

rwcarlsen · 2020-11-12T16:02:52Z

test/tests/postprocessors/interface_value/interface_integral_variable_value_postprocessor_fv.i

+[Postprocessors]
+  [./diffusivity_average]
+    type = InterfaceIntegralVariableValuePostprocessor
+    interface_value_type = average
+    variable = diffusivity_1
+    neighbor_variable = diffusivity_2
+    execute_on = TIMESTEP_END
+    boundary = 'interface'
+  [../]
+  [./diffusivity_jump_primary_secondary]
+    type = InterfaceIntegralVariableValuePostprocessor
+    interface_value_type = jump_primary_minus_secondary
+    variable = diffusivity_1
+    neighbor_variable = diffusivity_2
+    execute_on = TIMESTEP_END
+    boundary = 'interface'
+  [../]
+  [./diffusivity_jump_secondary_primary]
+    type = InterfaceIntegralVariableValuePostprocessor
+    interface_value_type = jump_secondary_minus_primary
+    variable = diffusivity_1
+    neighbor_variable = diffusivity_2


I think this input file can be combined with the average variable value postprocessor one. Just do type=${pp_type} and then have a top-level pp_type = foo parameter that you then set in the test spec file via the cli_args param to be either InterfaceIntegralVariableValuePostprocessor or InterfaceAverageVariableValuePostrocessor.

ok done.
I also did the same for the FE tests for consistency. Two less input files to download!

rwcarlsen · 2020-11-12T16:09:27Z

test/tests/userobjects/layered_integral/tests

@@ -28,7 +28,15 @@
      input = 'average_sample.i'
      exodiff = 'average_sample_out.e'

-      detail = 'as an average calculation.'
+      detail = 'as an average calculation,'


I do not understand what the detail writer on these tests was trying to do. These detail fragments don't mesh well with the group prefix. @aeslaughter - are detail comments suppose to not "finish" the group sentence each? Why do these end with "with" and "and"?

The requirement just needs to be cleaned up. I tend to write them as one complete sentence if possible.

requirement = "The system shall output data the" detail = "ExodusII," detail = "CSV, and" detail = "JSON formats."

This will render as:

The system shall output data the
(a) ExodusII,
(b) CSV, and
(c) JSON formats.

the way I wrote the requirements should work either way?

GiudGiud · 2020-11-12T20:18:15Z

ok lemme clean the mess with the gold. Ideally, I shouldnt need to re-gold those two I changed

Use command line arguments to simplify interface value tests, idaholab#16099

GiudGiud · 2020-11-12T20:30:50Z

Ok should be good now, aside from the "detail" thing

rwcarlsen · 2020-11-12T20:52:39Z

I'll wait for tests to finish before merging.

GiudGiud · 2020-11-16T02:23:43Z

I don't think the test failures have to do with this PR btw. Am I wrong?

moosebuild · 2020-11-16T15:52:53Z

Job App tests (Non Conda) on 21e9e5a : invalidated by @GiudGiud

Testing re-running a test

moosebuild · 2020-11-16T15:54:11Z

Job Private App tests on 21e9e5a : invalidated by @GiudGiud

Why are Relap7 and thm failing

moosebuild · 2020-11-16T15:56:33Z

Job Mac Test on 21e9e5a : invalidated by @rwcarlsen

rwcarlsen · 2020-11-16T18:13:15Z

There are 2 failing mastodon tests that look like they might be related to this. Mastodon lives on idaholab github. Why don't you check it out and see if you can reproduce the issue and confirm whether this PR causes it.

GiudGiud · 2020-11-16T20:25:04Z

They fail on my workstation with devel moose.
fv in those tests stand for fluid viscous not finite volume btw

GiudGiud added 4 commits November 5, 2020 20:22

Set variable dependency using MooseVariableFIeld, adresses idaholab#1…

dce7b1a

…6099

Use field variables in two transfers, add postprocessor FV tests, efs i…

eacc0fe

…daholab#16099

Use MooseVariableField in PointSamplerBase, add test, fix some typos, i…

d7750c9

…daholab#16099

Add test for MultiAppVariableValueSampleTransfer for FV variables, id…

6ab54ad

…aholab#16099

GiudGiud changed the title ~~Pr fv objects~~ Enable multiple objects, postprocs, transfers for FV variables Nov 8, 2020

GiudGiud added 2 commits November 7, 2020 19:53

Moose precheck syntax fix on FEProblembase header

f9c0a21

Use MooseVariableFieldBase instead of MooseVariableField<Real>. Add t…

6e81f25

…est for ElementVariableValuePP. idaholab#16099

moosebuild syntax fix

37a7b36

aeslaughter assigned rwcarlsen Nov 9, 2020

Add tests for FV user objects compatibility, ref idaholab#16099

6a3b037

Skip new tests using FV with stateful materials, refs idaholab#16099 i…

6bc998e

…daholab#16149

moosebuild added the PR: Failed but allowed label Nov 11, 2020

rwcarlsen suggested changes Nov 12, 2020

View reviewed changes

moosebuild removed the PR: Failed but allowed label Nov 12, 2020

GiudGiud force-pushed the PR_FV_objects branch from 8000835 to bcddbab Compare November 12, 2020 19:34

Address @rwcarlsen review

21e9e5a

Use command line arguments to simplify interface value tests, idaholab#16099

GiudGiud force-pushed the PR_FV_objects branch from bcddbab to 21e9e5a Compare November 12, 2020 20:29

rwcarlsen approved these changes Nov 12, 2020

View reviewed changes

moosebuild added the PR: Failed but allowed label Nov 16, 2020

rwcarlsen merged commit 9455797 into idaholab:next Nov 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable multiple objects, postprocs, transfers for FV variables #16115

Enable multiple objects, postprocs, transfers for FV variables #16115

GiudGiud commented Nov 8, 2020 •

edited

moosebuild commented Nov 9, 2020

rwcarlsen commented Nov 9, 2020

GiudGiud commented Nov 9, 2020 •

edited

rwcarlsen commented Nov 10, 2020

GiudGiud commented Nov 11, 2020 •

edited

moosebuild commented Nov 11, 2020 •

edited

rwcarlsen left a comment

rwcarlsen Nov 12, 2020

GiudGiud Nov 12, 2020

rwcarlsen Nov 12, 2020

GiudGiud Nov 12, 2020

rwcarlsen Nov 12, 2020

aeslaughter Nov 13, 2020

GiudGiud Nov 16, 2020

GiudGiud commented Nov 12, 2020

GiudGiud commented Nov 12, 2020

rwcarlsen commented Nov 12, 2020

GiudGiud commented Nov 16, 2020

moosebuild commented Nov 16, 2020

moosebuild commented Nov 16, 2020

moosebuild commented Nov 16, 2020

rwcarlsen commented Nov 16, 2020

GiudGiud commented Nov 16, 2020

Enable multiple objects, postprocs, transfers for FV variables #16115

Enable multiple objects, postprocs, transfers for FV variables #16115

Conversation

GiudGiud commented Nov 8, 2020 • edited

moosebuild commented Nov 9, 2020

rwcarlsen commented Nov 9, 2020

GiudGiud commented Nov 9, 2020 • edited

rwcarlsen commented Nov 10, 2020

GiudGiud commented Nov 11, 2020 • edited

moosebuild commented Nov 11, 2020 • edited

rwcarlsen left a comment

Choose a reason for hiding this comment

rwcarlsen Nov 12, 2020

Choose a reason for hiding this comment

GiudGiud Nov 12, 2020

Choose a reason for hiding this comment

rwcarlsen Nov 12, 2020

Choose a reason for hiding this comment

GiudGiud Nov 12, 2020

Choose a reason for hiding this comment

rwcarlsen Nov 12, 2020

Choose a reason for hiding this comment

aeslaughter Nov 13, 2020

Choose a reason for hiding this comment

GiudGiud Nov 16, 2020

Choose a reason for hiding this comment

GiudGiud commented Nov 12, 2020

GiudGiud commented Nov 12, 2020

rwcarlsen commented Nov 12, 2020

GiudGiud commented Nov 16, 2020

moosebuild commented Nov 16, 2020

moosebuild commented Nov 16, 2020

moosebuild commented Nov 16, 2020

rwcarlsen commented Nov 16, 2020

GiudGiud commented Nov 16, 2020

GiudGiud commented Nov 8, 2020 •

edited

GiudGiud commented Nov 9, 2020 •

edited

GiudGiud commented Nov 11, 2020 •

edited

moosebuild commented Nov 11, 2020 •

edited