Refactoring problem creation to allow custom actions to create special problems #12060

YaqiWang · 2018-08-30T21:34:37Z

Close #12002.

I still need to add tests and docs. But want to test this and let you see what is in my mind. Tag @permcody @andrsd .

YaqiWang · 2018-08-30T22:08:32Z

I do not understand why this PR runs fine on my local mac but fails on test boxes with this weird null feproblem pointer error...

moosebuild · 2018-08-30T22:10:00Z

Job Documentation on 060981b wanted to post the following:

View the site here

This comment will be updated on new commits.

YaqiWang · 2018-08-30T22:12:03Z

oh, crap, I forgot check in two new files...

moosebuild · 2018-08-30T22:16:29Z

Job Precheck on b61de47 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 http://mooseframework.inl.gov/docs/PRs/12060/style.patch | git apply -v

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

git clang-format ccf22351fc1719a0015c5eeaaa952f302f27eaeb

permcody · 2018-08-30T22:19:38Z

You might consider grabbing the tests that I created. You'll have to modify the CreateSpecialProblemAction so that it just creates a problem but the rest of that should be good.

YaqiWang · 2018-08-30T22:37:49Z

Yes, working on that. I also need to add all tests to meet the requirements you proposed.

YaqiWang · 2018-08-30T23:10:25Z

This should be it. We possibly still need to polish it a little. The other few requirements without custom action has been tested by other tests so we do not need to test them here.

YaqiWang · 2018-08-30T23:22:56Z

framework/src/base/MooseApp.C

-    // Set the object parameters
-    InputParameters & params = action->getObjectParams();
-    params.set<bool>("solve") = false;
+        _action_factory.create("CreateProblemDefaultAction", "Problem", action_params));


strange, this should fail because CreateProblemDefaultAction is no longer a moose object action. I will spend some time this weekend to polish this PR...

YaqiWang · 2018-08-31T17:05:35Z

Rattlesnake fails because it adds a new action task after create_problem, which should be create_problem_complete with this PR. We will have to merge this first to patch Rattlesnake. I do not have access to Bighorn, looks like the error is real and I guess it is caused by kernel_coverage_check being moved to FEProblemBase with default value true. The tests can probably fixed by add Problem/kernel_coverage_check=false or change the default value to false in FEProblemBase, but it could be better to check the code to see if there is something not very right.

permcody · 2018-09-04T15:59:13Z

I don't immediately see why this is failing. My PR worked fine with Bighorn without changing any input files. The kernel coverage check should be fine being moved up into the Problem class.

YaqiWang · 2018-09-04T16:02:16Z

Right, even the default value in FEProblemBase before is false, it is reset to true by the CreateProblemAction before.

permcody · 2018-09-04T16:03:04Z

framework/src/base/Moose.C

@@ -218,6 +222,9 @@ addActionTypes(Syntax & syntax)
                           "(setup_mesh_complete)"
                           "(determine_system_type)"
                           "(create_problem)"
+                           "(create_problem_custom)"
+                           "(create_problem_default)"
+                           "(create_problem_complete)"


So if I create a [Problem] block, it seems like the first action would always create the problem with this design. The custom task would run after that and wouldn't take effect. How does this work?

If the design is to allow parameters from the [Problem] to be applied to a custom problem, I don't see how this is working. Should custom be first?

permcody · 2018-09-04T16:05:48Z

test/src/actions/CreateSpecialProblemAction.C

+  if (_problem)
+  {
+    // problem has been created by CreateProblemAction
+    if (_problem->type() != "MooseTestProblem")


Hmm, this looks right to me.

Yes, this particular action answers your question before. I think we want the input syntax to win always, right?

This also means that the custom action needs to follow the similar pattern here for creating problems.

YaqiWang · 2018-09-04T17:08:00Z

Another thing I want to bring up is that maybe we want a normal action like SetProblemParametersAction with input syntax like [ProblemParameters] on create_problem_complete task, that accepts some of the parameters in FEProblemBase to give users second chance to change problem parameters. I can see its usage with custom actions, where users doe not have to use their own syntax to set problem parameters correctly but prefer to use this action to set them. If you think this is ok, I can add a commit on top to add this action.

andrsd · 2018-09-04T17:19:57Z

I still think that problem parameters should be with the type = parameter. That's is the syntax we use. Think of a kernel of a custom type with its own parameters, the syntax is:

type = CustomKernel
param1 = something
param2 = something

Now because, the CustomKernel inherits from Kernel all these things have common parameters like variable, block, etc. The same holds for FEProblemBase-derived classes. There should not be an exception to this...

permcody · 2018-09-04T17:23:03Z

I agree with @andrsd. This was one of the requirements for this redesign: #12055 (comment)

We already have a [Problem] block, having a [ProblemParameters] block is very inconsistent with MOOSE. My PR solved this, it was possible to have parameters for the problem in the [Problem] block even when that block wasn't responsible for creating the actual problem (e.g. your custom problem creation). Now admittedly, my PR needs one more patch to make that work properly (and obviously a test), but it will work without requiring this separate block. It would be nice if you could figure out how to make this work if it's possible at all with your current design.

YaqiWang · 2018-09-04T17:45:21Z

A simple approach is to modify CreateProblemAction in my PR a little:

create problem only when type is set by user;
create a function like void applyProblemParameters(InputParameters & params) const that other actions can call to add parameters.

Then CreateProblemDefaultAction or custom actions can use this function to create the problem. How does this sound?

YaqiWang · 2018-09-04T18:28:30Z

Just added one more commit. I think this works better. InputParameters has already a function applyParameters which does exactly what we want here. Getting CreateProblemAction is a little lengthy and code are duplicated, maybe we should create a function in ActionWarehouse like

  template <class T>
  const T * getActionOnTask(const std::string & task)

I can add this during addressing your possible other comments.

YaqiWang · 2018-09-04T20:03:51Z

@permcody can you review this? I want this to be pushed in and removed from my plate soon.

permcody · 2018-09-04T22:32:43Z

This is looking pretty good. I looked at Bighorn and it can be patched easily. I have one question on design. Do we really need all these tasks? determine_system_type, create_problem, create_problem_custom, create_problem_default, create_problem_complete.

In determine_system_type you are stashing info in the App which you pull later to create the problem. However, I think this step can simply be skipped since the very next action is create_problem. There's nothing in-between, not even the custom problem step.

Can the custom problem just fire on create_problem? It seems like yes because we have the apply parameters stuff in here, I could be convinced otherwise if the logic gets more complicated.

YaqiWang · 2018-09-04T22:46:11Z

CreateProblemAction uses MooseApp::useNonlinear(). But we cannot set useNonlinear there because it may not be created by the parser. So we will have to have a task before create_problem. On the other hand CreateProblemDefaultAction has to be fired after CreateProblemAction, which means we will have to have create_problem_default task.

create_problem_custom also has to exist because we allow Problem/type to create a special problem together with a custom action, refer to the implementation of CreateSpecialProblemAction, which requires the correct ordering of MOOSE CreateProblemAction and custom action. At this moment create_problem_complete is not used but I think it might be a good idea to keep it for the future.

YaqiWang · 2018-09-04T22:50:11Z

A side note is that useNonlinear is used to add different systems in FEProblem for our old eigenvalue calculation. It possibly will be removed thus determine_system_type will not be needed. But currently Rattlesnake is using the old eigenvalue capability intensively and new system is not completely ready yet.

YaqiWang · 2018-09-04T22:57:50Z

En, create_probem_custom can be removed if we check the existence of Problem/type instead of _problem. Thus we know we will be only having one action creating the problem other than creating the default problem.

permcody

Ok, my only nitpick is that new method. I don't like that it only gives back one Action. I'd suggest returning the whole container, or just use the existing iterators to get back the items one at a time. I'd rather not create a new interface to maintain in the ActionWarehouse if it can be avoided.

permcody · 2018-09-05T18:55:26Z

framework/include/actions/ActionWarehouse.h

+   * @return The action pointer. Null means that such an action does not exist.
+   */
+  template <class T>
+  const T * getActionByTask(const std::string & task)


This method isn't very useful. There are often several Actions for a single task with the same type. This only return the first one. Why not return a container of all the converted ones? A better solution might be to just use the existing iterator (which can be captured and turned into a range trivially) and just loop over them yourself on the receiving side.

Right, I was not thinking something like AddMaterialAction, although the use case in this PR always only have one action. Without this helper function, the same functionality has to be repeated several places, like checking the existence of a task, loop through actions on the task. How about I have a check in this function to make sure there is only one action, otherwise throw an error?

How does this look like?

/** * Retrieve an action on a specific task with its type. * Error will be thrown if more than one action is found. * @param name The task name. * @return The action pointer. Null means that such an action does not exist. */ template <class T> const T * getActionByTask(const std::string & task) { const auto it = _action_blocks.find(task); if (it == _action_blocks.end()) return nullptr; T * p = nullptr; for (const auto & action : it->second) { T * tp = dynamic_cast<T *>(action); if (tp) { if (p) mooseError("More than one actions have been detected in getActionByTask"); else p = tp; } } return p; }

YaqiWang · 2018-09-05T22:12:02Z

@permcody I hope you are satisfied. I have been waiting on this too long...

permcody · 2018-09-06T14:15:21Z

@andrsd - This will require another Relap-7 patch. Please review. I already have a Bighorn patch ready.

framework/include/actions/ActionWarehouse.h

framework/include/problems/FEProblemBase.h

framework/src/actions/CreateProblemAction.C

framework/src/actions/CreateProblemDefaultAction.C

framework/src/problems/DumpObjectsProblem.C

framework/src/problems/FEProblemBase.C

andrsd · 2018-09-06T14:59:01Z

This will require another Relap-7 patch.

The patch is now ready.

Also, thanks for patching bighorn.

YaqiWang · 2018-09-06T14:59:49Z

Thanks, I will address the comments shortly. Just a question to @permcody on factory creating object.

…l problems idaholab#12002

…ab#12002

YaqiWang · 2018-09-06T16:50:06Z

@andrsd Your comments are addressed. I think this is ready. Thanks guys.

andrsd · 2018-09-06T18:35:16Z

I am merging this.

YaqiWang · 2018-09-06T18:44:02Z

Thanks!

YaqiWang · 2018-09-06T20:07:45Z

Both Rattlesnake and Mammoth have been patched. The fix are all on their devel, because from devel to master is controlled by Civet and Civet will not do it due to App test failure. Looks R7 and Bighorn also have been patched. We will need to do submodule update in Sabertooth and Crab. Then we will back to normal.

YaqiWang · 2018-09-06T20:08:06Z

Tag @andrsd and @permcody.

YaqiWang force-pushed the redesign_create_problem branch from 0bbd1ce to ab844d0 Compare August 30, 2018 21:39

YaqiWang force-pushed the redesign_create_problem branch 3 times, most recently from b61de47 to 2b3c28f Compare August 30, 2018 22:16

YaqiWang force-pushed the redesign_create_problem branch from 2b3c28f to 3f025ce Compare August 30, 2018 23:06

YaqiWang mentioned this pull request Aug 30, 2018

Allow user to override problem creation #12055

Closed

YaqiWang commented Aug 30, 2018

View reviewed changes

YaqiWang force-pushed the redesign_create_problem branch 2 times, most recently from f8a5d36 to e162b60 Compare August 31, 2018 16:25

permcody reviewed Sep 4, 2018

View reviewed changes

YaqiWang force-pushed the redesign_create_problem branch from 7a0586b to 313d6aa Compare September 4, 2018 18:49

YaqiWang force-pushed the redesign_create_problem branch 2 times, most recently from 003cff6 to e60d038 Compare September 5, 2018 16:41

permcody requested changes Sep 5, 2018

View reviewed changes

YaqiWang force-pushed the redesign_create_problem branch from e60d038 to 1bd5636 Compare September 5, 2018 20:29

permcody requested a review from andrsd September 6, 2018 14:15

permcody previously approved these changes Sep 6, 2018

View reviewed changes

andrsd suggested changes Sep 6, 2018

View reviewed changes

add a utility function in ActionWarehouse to be used by idaholab#12002

750eca1

YaqiWang dismissed permcody’s stale review via 060981b September 6, 2018 15:17

YaqiWang force-pushed the redesign_create_problem branch from 1bd5636 to 060981b Compare September 6, 2018 15:17

YaqiWang added 6 commits September 6, 2018 09:23

refactoring problem creation to allow custom actions to create specia…

da77e75

…l problems idaholab#12002

merge DetermineSystemType into CreateProblemDefaultAction idaholab#12002

3f7dea2

add tests for adding custom problem programmatically idaholab#12002

0292c2b

add documentation on problems by Cody idaholab#12002

b9598ea

allow custom actions to take parameters in Problem input block idahol…

2ea4027

…ab#12002

add a const version of a function idaholab#12002

060981b

andrsd approved these changes Sep 6, 2018

View reviewed changes

andrsd merged commit b614a5e into idaholab:devel Sep 6, 2018

YaqiWang deleted the redesign_create_problem branch September 6, 2018 20:11

Refactoring problem creation to allow custom actions to create special problems #12060

Refactoring problem creation to allow custom actions to create special problems #12060

Conversation

YaqiWang commented Aug 30, 2018

YaqiWang commented Aug 30, 2018

moosebuild commented Aug 30, 2018 • edited

YaqiWang commented Aug 30, 2018

moosebuild commented Aug 30, 2018

permcody commented Aug 30, 2018

YaqiWang commented Aug 30, 2018

YaqiWang commented Aug 30, 2018 • edited

Choose a reason for hiding this comment

YaqiWang commented Aug 31, 2018 • edited

permcody commented Sep 4, 2018

YaqiWang commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaqiWang commented Sep 4, 2018

andrsd commented Sep 4, 2018

permcody commented Sep 4, 2018

YaqiWang commented Sep 4, 2018

YaqiWang commented Sep 4, 2018 • edited

YaqiWang commented Sep 4, 2018

permcody commented Sep 4, 2018

YaqiWang commented Sep 4, 2018 • edited

YaqiWang commented Sep 4, 2018

YaqiWang commented Sep 4, 2018

permcody left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaqiWang commented Sep 5, 2018

permcody commented Sep 6, 2018

andrsd commented Sep 6, 2018 • edited

YaqiWang commented Sep 6, 2018

YaqiWang commented Sep 6, 2018

andrsd commented Sep 6, 2018

YaqiWang commented Sep 6, 2018

YaqiWang commented Sep 6, 2018

YaqiWang commented Sep 6, 2018

moosebuild commented Aug 30, 2018 •

edited

YaqiWang commented Aug 30, 2018 •

edited

YaqiWang commented Aug 31, 2018 •

edited

YaqiWang commented Sep 4, 2018 •

edited

YaqiWang commented Sep 4, 2018 •

edited

andrsd commented Sep 6, 2018 •

edited