Add a parameter object on the system, use it in derived classes #4007

GiudGiud · 2024-11-12T22:38:20Z

If this is too much, I could scale this down to just the nonlinear_implicit_system. That's all I need.
We could consider a getParam routine that does this 3-liner operation instead.

src/systems/frequency_system.C

roystgnr · 2024-11-13T19:39:30Z

I like this, and I wouldn't scale it down, though I'm crossing my fingers hoping we aren't breaking any user subclasses that also thought parameters was a good name for a local variable.

GiudGiud · 2024-11-14T18:17:58Z

@roystgnr do you have thoughts on testing this? Do you want to rely on our use in MOOSE for testing? This will be used for new Convergence objects for each nonlinear system in a multi-system context.

Or should there be an example? Or some other form of testing?

moosebuild · 2024-11-14T19:32:24Z

Job Coverage, step Generate coverage on f3746dc wanted to post the following:

Coverage

	5435e2	#4007 f3746d
	Total	Total	+/-	New
Rate	62.36%	62.34%	-0.02%	37.88%
Hits	72657	72668	+11	50
Misses	43857	43895	+38	82

Diff coverage report

Full coverage report

Warnings

New new line coverage rate 37.88% is less than the suggested 90.0%

This comment will be updated on new commits.

roystgnr · 2024-11-14T19:41:23Z

I hate relying on MOOSE tests for coverage, and IIRC we deliberately exclude MOOSE tests from our coverage stats for that reason. It's hard to come up with a good unit test here, but ... maybe set up a problem that should require a lot of linear iterations, but then set a system parameter that holds us to a tiny max number of iterations, and check the number of iterations reported after a solve()? We can't assert that it's equal to our max iterations (unless we set max=1? that wouldn't be crazy) lest we break with -pc_type lu, but we can at least assert that it doesn't exceed our max iterations.

…it system refs libMesh#4006

GiudGiud · 2024-11-15T02:15:01Z

I added a test that should show that parameters are read from the system rather than the equation system when both are present. It's not the best either because if we changed the name of the parameter of the system, the test would pass even through the change does nothing.
If we want 100% coverage we'll need a test for every system and every parameter?

GiudGiud · 2024-11-15T14:22:24Z

ok made a little mistake. going to push it again soon

GiudGiud · 2024-11-22T17:01:39Z

@roystgnr

src/systems/frequency_system.C

roystgnr · 2024-11-26T22:41:22Z

src/systems/implicit_system.C

+    return std::make_pair(this->get_equation_systems().parameters.get<unsigned int>("linear solver maximum iterations"),
+                          this->get_equation_systems().parameters.get<Real>("linear solver tolerance"));
+  else
+    libmesh_error_msg("ERROR: Insufficient linear solver parameters");


This isn't what we want to do, is it? If we override one parameter for a particular system, we should still just default to the EquationSystems parameter value for the other.

ah that works.
I was thinking of forcing the user to be consistent between where each parameter comes from

it s probably more flexible this other way

roystgnr · 2024-11-26T22:45:22Z

src/systems/frequency_system.C

+  const Real tol            = parameters.have_parameter<Real>("linear solver tolerance") ?
+    double(parameters.get<Real>("linear solver tolerance")) :
    es.parameters.get<Real>("linear solver tolerance");
-  const unsigned int maxits =
+  const unsigned int maxits = parameters.have_parameter<unsigned int>("linear solver maximum iterations") ?
+    parameters.get<unsigned int>("linear solver maximum iterations") :
    es.parameters.get<unsigned int>("linear solver maximum iterations");


As long as we have to change all these anyway, wouldn't auto [maxits, tol] = this->get_linear_solve_parameters(); work? We can't do that in EigenSystem because it doesn't inherit from ImplicitSystem, but it seems like it ought to work here, and in LinearImplicitSystem, and (albeit for just these two parameters) in NonlinearImplicitSystem.

roystgnr · 2024-11-26T23:18:15Z

tests/systems/systems_test.C

+
+    // We set the solution to be 1 everywhere, so the final l1 norm of the
+    // solution is the product of the number of variables and number of nodes.
+    Real ref_l1_norm = static_cast<Real>(mesh.n_nodes() * li_system.n_vars());


Instead of using enough iterations to converge and then testing convergence, we should be using too few iterations to converge (so we stop at the max iteration count) and testing to make sure we're at the specified max iteration count.

roystgnr · 2024-11-26T23:19:53Z

tests/systems/systems_test.C

@@ -1256,6 +1257,78 @@ public:
    LIBMESH_ASSERT_FP_EQUAL(system.solution->l1_norm(), ref_l1_norm, TOLERANCE*TOLERANCE);
  }

+
+  void testSetSystemParameterOverEquationSystem()


This test isn't currently being run. You need to add a CPPUNIT_TEST( testSystemParameterOverEquationSystems ); macro invocation with the others at the start of the class.

roystgnr · 2024-11-27T16:42:17Z

We might be out of luck for the nonlinear test; that has no way to tell you the final step's linear iteration count, only the nonlinear iteration count. The linear test ought to be sufficient, except that you'll have to use 5u instead of 5 there, because cppunit isn't clever enough to cope with tests that mix different types, even as obviously-mixable types as int and unsigned int.

GiudGiud · 2024-11-27T22:01:25Z

for the nonlienar system I kept my old test essentially, checking another parameter rather than num linear iterations

- centralize change in get_linear_solve_parameters - enable the test - use the number of iterations for the test

GiudGiud force-pushed the PR_nl_params branch 2 times, most recently from 94329c3 to 38655d9 Compare November 13, 2024 00:19

roystgnr reviewed Nov 13, 2024

View reviewed changes

src/systems/frequency_system.C Outdated Show resolved Hide resolved

roystgnr reviewed Nov 13, 2024

View reviewed changes

src/systems/frequency_system.C Outdated Show resolved Hide resolved

GiudGiud force-pushed the PR_nl_params branch 3 times, most recently from 6b546ae to 4d281ca Compare November 14, 2024 16:02

GiudGiud added 2 commits November 14, 2024 17:29

Add a parameter object on the system, use it for the nonlinear implic…

7b8450f

…it system refs libMesh#4006

Rename local variables shadowing new attribute

65da3a6

GiudGiud force-pushed the PR_nl_params branch 3 times, most recently from 3620605 to 5fb6732 Compare November 15, 2024 01:33

GiudGiud force-pushed the PR_nl_params branch from 5fb6732 to 730214d Compare November 15, 2024 14:34

Add a test using system parameters over equation system parameters

f3746dc

GiudGiud force-pushed the PR_nl_params branch from 730214d to f3746dc Compare November 15, 2024 14:58

GiudGiud mentioned this pull request Nov 18, 2024

Add multi-system to FEProblemSolve and deploy on Physics syntax idaholab/moose#29021

Merged

GiudGiud requested a review from roystgnr November 18, 2024 18:52

roystgnr requested changes Nov 26, 2024

View reviewed changes

roystgnr reviewed Nov 26, 2024

View reviewed changes

GiudGiud force-pushed the PR_nl_params branch from 7680964 to 329cfd4 Compare November 27, 2024 14:34

GiudGiud force-pushed the PR_nl_params branch from 329cfd4 to 6b51ba0 Compare November 27, 2024 18:21

GiudGiud force-pushed the PR_nl_params branch from 6b51ba0 to 69f1be1 Compare November 27, 2024 22:00

GiudGiud force-pushed the PR_nl_params branch from 69f1be1 to 02c160b Compare November 27, 2024 22:29

Address Roy's review:

862bcb5

- centralize change in get_linear_solve_parameters - enable the test - use the number of iterations for the test

GiudGiud force-pushed the PR_nl_params branch from 02c160b to 862bcb5 Compare November 28, 2024 14:10

Remove nonlinear system parameter test

cc9efd7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a parameter object on the system, use it in derived classes #4007

Add a parameter object on the system, use it in derived classes #4007

GiudGiud commented Nov 12, 2024 •

edited

Loading

roystgnr commented Nov 13, 2024

GiudGiud commented Nov 14, 2024

moosebuild commented Nov 14, 2024 •

edited

Loading

roystgnr commented Nov 14, 2024

GiudGiud commented Nov 15, 2024

GiudGiud commented Nov 15, 2024

GiudGiud commented Nov 22, 2024

roystgnr Nov 26, 2024

GiudGiud Nov 27, 2024

roystgnr Nov 26, 2024

roystgnr Nov 26, 2024

GiudGiud Nov 27, 2024

roystgnr Nov 26, 2024

roystgnr commented Nov 27, 2024

GiudGiud commented Nov 27, 2024

Add a parameter object on the system, use it in derived classes #4007

Are you sure you want to change the base?

Add a parameter object on the system, use it in derived classes #4007

Conversation

GiudGiud commented Nov 12, 2024 • edited Loading

roystgnr commented Nov 13, 2024

GiudGiud commented Nov 14, 2024

moosebuild commented Nov 14, 2024 • edited Loading

Coverage

Warnings

roystgnr commented Nov 14, 2024

GiudGiud commented Nov 15, 2024

GiudGiud commented Nov 15, 2024

GiudGiud commented Nov 22, 2024

roystgnr Nov 26, 2024

Choose a reason for hiding this comment

GiudGiud Nov 27, 2024

Choose a reason for hiding this comment

roystgnr Nov 26, 2024

Choose a reason for hiding this comment

roystgnr Nov 26, 2024

Choose a reason for hiding this comment

GiudGiud Nov 27, 2024

Choose a reason for hiding this comment

roystgnr Nov 26, 2024

Choose a reason for hiding this comment

roystgnr commented Nov 27, 2024

GiudGiud commented Nov 27, 2024

GiudGiud commented Nov 12, 2024 •

edited

Loading

moosebuild commented Nov 14, 2024 •

edited

Loading