Add between multiapp (sibling) copy transfer #19676

GiudGiud · 2021-12-22T01:39:09Z

Early PR to get feedback from @fdkong

And to see if the test suite sees issues with it.

The main limitation of this (beyond all the limitations of copy transfer compared to projection & conservative transfers), I think, is that it cant do anything if different ranks own each sibling.
Maybe there is a way around it though, since copy transfers can assume a lot of things are true, because the same mesh is involved

Also let's say I have two different kinds of multiapps, with three instances for each, and I m running this on 2 processes for the main app. Is the allocation going to be:

process 1: subapp1: instances 1&2, subapp2: instances 1&2. Process2: instances 3 for both subapps. (Works for this PR!)
OR
process 1: subapp1: instances 1&2, subapp2: instances 1. Process2: subapp1 instance 3, subapp2 instances 2&3 (doesnt work)

The to_multi_app parameter that is only used for between_multiapp transfers is not a good name. Maybe I should call it "other_multiapp". It's hard to find a name that doesnt convey "to" or "from" in the TO_MULTIAPP and FROM_MULTIAPP contexts, yet do convey it's a target in between_multiapp.

Another difficulty is when do we do these "between" transfers. Before the apps are ran? After they are ran? In between (what I d want actually) ???

App patches:

moosebuild · 2021-12-22T02:48:44Z

Job Documentation on d8ba4c0 wanted to post the following:

View the site here

This comment will be updated on new commits.

moosebuild · 2021-12-22T04:27:42Z

Job Coverage on d8ba4c0 wanted to post the following:

Framework coverage

	c90b1f	#19676 d8ba4c
	Total	Total	+/-	New
Rate	82.56%	82.55%	-0.01%	86.82%
Hits	73757	73913	+156	382
Misses	15581	15627	+46	58

Diff coverage report

Full coverage report

Modules coverage

Functional expansion tools

	c90b1f	#19676 d8ba4c
	Total	Total	+/-	New
Rate	83.09%	83.05%	-0.04%	81.25%
Hits	924	926	+2	13
Misses	188	189	+1	3

Diff coverage report

Full coverage report

Level set

	c90b1f	#19676 d8ba4c
	Total	Total	+/-	New
Rate	88.12%	88.15%	+0.03%	100.00%
Hits	319	320	+1	9
Misses	43	43	-	0

Diff coverage report

Full coverage report

Stochastic tools

	c90b1f	#19676 d8ba4c
	Total	Total	+/-	New
Rate	89.90%	89.84%	-0.06%	83.05%
Hits	4425	4432	+7	49
Misses	497	501	+4	10

Diff coverage report

Full coverage report

Full coverage reports

Reports

Warnings

framework new line coverage rate 86.82% is less than the suggested 90.0%
functional_expansion_tools new line coverage rate 81.25% is less than the suggested 90.0%
stochastic_tools new line coverage rate 83.05% is less than the suggested 90.0%

This comment will be updated on new commits.

loganharbour

Commenting to enable notifications; I've got a few ideas for this but haven't had the chance to get them down.

fdkong · 2022-01-06T00:37:57Z

Also let's say I have two different kinds of multiapps, with three instances for each, and I m running this on 2 processes for the main app. Is the allocation going to be:

process 1: subapp1: instances 1&2, subapp2: instances 1&2. Process2: instances 3 for both subapps. (Works for this PR!)
OR
process 1: subapp1: instances 1&2, subapp2: instances 1. Process2: subapp1 instance 3, subapp2 instances 2&3 (doesnt work)

I think we need to talk about two cases here:

Within the same multiapp context, we transfer from a subapp to another subpp, and these apps are on the same level.
Between multiple multiapp contexts, we transfer a subapp of this multiapp to another subpp of another multiapp.

fdkong · 2022-01-06T00:45:56Z

The to_multi_app parameter that is only used for between_multiapp transfers is not a good name. Maybe I should call it "other_multiapp". It's hard to find a name that doesnt convey "to" or "from" in the TO_MULTIAPP and FROM_MULTIAPP contexts, yet do convey it's a target in between_multiapp.

I think we should keep using the same TO_MULTIAPP and FROM_MULTIAPP. We think the target is the master in the current code base when we say "FROM_MULTIAPP." We think the source is the master when we say "TO_MULTIAPP."

We should not make that assumption anymore for this "between-MULTIAPP." The between case set both FROM_MULTIAPP and TO_MULTIAPP to be true

fdkong · 2022-01-06T00:47:21Z

Another difficulty is when do we do these "between" transfers. Before the apps are ran? After they are ran? In between (what I d want actually) ???

We might introduce extra "execute_on" flags

GiudGiud · 2022-01-14T03:56:13Z

I think we want context 2. here, and assume that there is only one of each (or one per rank at least).

Otherwise, and in context 1 as well, we'd need to start numbering subapps of a given multiapp, and be very precise what is moved where

GiudGiud · 2022-02-11T03:07:16Z

@fdkong I did the parameter renaming for Transfer and MultiAppTransfer (but not all the subclasses yet)
If you could have a look so we can coordinate on those changes for sibling transfers

GiudGiud · 2022-02-18T07:10:53Z

Should pass test now.
using singular to_ and from_multiapp.

I think I ll try to grab a few of the other easy transfers then this will be good to go

…ectional transfer, refs idaholab#19451

…19451 also for an input in level set module

…rasnfers. refs idaholab#19451

…holab#19451

…daholab#19451

…rectional parameters, refs idaholab#19451

Fixup new tests added during PR

…velopers to use them Refs idaholab#19451 TODO: - adapt modules - app patches

moosebuild · 2022-03-29T08:06:15Z

Job Precheck on dd1dc50 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/19676/style.patch | git apply -v

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

git clang-format c90b1f04c38b5fa8c35d8b16e7ca4ae7c7ad7115

Clang format

Resolved

lindsayad · 2022-04-15T20:26:26Z

framework/src/problems/FEProblemBase.C

+    if (parameters.isParamValid("multi_app"))
+      multiapp = getMultiApp(parameters.get<MultiAppName>("multi_app"));
+    else if (parameters.isParamValid("from_multi_app"))
+      multiapp = getMultiApp(parameters.get<MultiAppName>("from_multi_app"));
+    else if (parameters.isParamValid("to_multi_app"))
+      multiapp = getMultiApp(parameters.get<MultiAppName>("to_multi_app"));
+    else
+      mooseError("Should not reach here");


we need to have a better error message here. A user specified to_multiapp as their parameter name and got the "Should not reach here" error

oh wait ... it's just that these parameter names here are actually wrong. The user had specified the right parameter name in to_multiapp

It might be helpful to output users' input

my bad.
How are they hitting this? All the apps were moved to the new system

well actually, how are they hitting this just now?
is EXEC_SAME_AS_MULTIAPP just not tested?

I misread the situation. See #20803

Refs idaholab#19676

Also fix typos: 'directions' to 'direction' towards bottom of `MultiAppTransfer` constructor Refs idaholab#19676

permcody · 2022-10-11T07:49:39Z

Yes. This should be a MooseEnum for input file reasons (case, pick lists). As far as the backend, an enumeration makes a lot of sense just so you can see the actual names as written constants. I prefer the newer strong type C++ enumerations though over the older C types.

…

On Mon, Mar 21, 2022 at 2:53 PM Fande Kong ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In framework/src/problems/FEProblemBase.C <#19676 (comment)>: > @@ -4214,9 +4224,18 @@ void FEProblemBase::execMultiAppTransfers(ExecFlagType type, Transfer::DIRECTION direction) { bool to_multiapp = direction == MultiAppTransfer::TO_MULTIAPP; - std::string string_direction = to_multiapp ? " To " : " From "; - const MooseObjectWarehouse<Transfer> & wh = - to_multiapp ? _to_multi_app_transfers[type] : _from_multi_app_transfers[type]; + bool from_multiapp = direction == MultiAppTransfer::FROM_MULTIAPP; + std::string string_direction; + if (to_multiapp) + string_direction = " To "; + else if (from_multiapp) + string_direction = " From "; + else + string_direction = " between "; Should we use enum for the direction instead of string? We could move the following enum to the MooseTypes.h enum DIRECTION { TO_MULTIAPP, FROM_MULTIAPP FROM_MULTIAPP, BETWEEN_MULTIAPP }; — Reply to this email directly, view it on GitHub <#19676 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXFOIHEARHST5OLR2WNVL3VBDOWNANCNFSM5KROOMJA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

GiudGiud requested a review from fdkong as a code owner December 22, 2021 01:39

GiudGiud changed the title ~~Add between multiapp (sibling) transfer~~ Add between multiapp (sibling) copy transfer Dec 22, 2021

GiudGiud force-pushed the PR_same_app branch from 5117850 to fea6cac Compare December 22, 2021 01:47

idaholab deleted a comment from moosebuild Dec 22, 2021

moosebuild added the PR: Failed but allowed label Dec 22, 2021

loganharbour assigned GiudGiud Jan 5, 2022

loganharbour reviewed Jan 5, 2022

View reviewed changes

GiudGiud force-pushed the PR_same_app branch from fea6cac to cc9747b Compare February 11, 2022 03:04

moosebuild removed the PR: Failed but allowed label Feb 11, 2022

GiudGiud force-pushed the PR_same_app branch from cc9747b to c5b37d8 Compare February 11, 2022 03:11

GiudGiud force-pushed the PR_same_app branch 10 times, most recently from 1797f10 to 1f42b40 Compare February 18, 2022 07:10

idaholab deleted a comment from moosebuild Feb 18, 2022

GiudGiud force-pushed the PR_same_app branch from 1f42b40 to 3ed5525 Compare February 18, 2022 16:26

GiudGiud and others added 10 commits March 29, 2022 00:57

Dont need to get both problems on first pass of getAppInfo for bi-dir…

7552b57

…ectional transfer, refs idaholab#19451

Fix deprecation issue in level_set transfer, refs idaholab#19451

855872f

Transition stochastic tools transfers in module tests, refs idaholab#…

06cc8fe

…19451 also for an input in level set module

Fix deprecation issue with suppressed parameter in stochastic tools t…

878ae3a

…rasnfers. refs idaholab#19451

Transition stochastic tools examples to new multiapp syntax, refs ida…

5ac6f83

…holab#19451

Transition combined stochastic examples to new multiapp syntax, refs i…

50d3fbd

…daholab#19451

Add exception checking for transfers for proviiding wrong multiapp di…

65a2aa9

…rectional parameters, refs idaholab#19451

Prepared MultiAppTransfer for "between" execution

22d21f0

Fixup new tests added during PR

Add deprecated attribute to avoid breaking next as hard

9567b6c

Add getters for multiapps, and make attribute private to force app de…

c28c477

…velopers to use them Refs idaholab#19451 TODO: - adapt modules - app patches

GiudGiud force-pushed the PR_same_app branch from 04da9bf to dd1dc50 Compare March 29, 2022 08:02

Use getters for multiapps in modules, refs idaholab#19451

d8ba4c0

Clang format

GiudGiud force-pushed the PR_same_app branch from dd1dc50 to d8ba4c0 Compare March 29, 2022 15:17

fdkong approved these changes Apr 4, 2022

View reviewed changes

GiudGiud merged commit d11575b into idaholab:next Apr 4, 2022

GiudGiud deleted the PR_same_app branch April 4, 2022 15:17

GiudGiud mentioned this pull request Apr 4, 2022

Adapt inputs and transfers to new MOOSE multiapp transfer syntax neams-th-coe/cardinal#355

Merged

cticenhour mentioned this pull request Apr 4, 2022

Various SQA documentation fixes #20693

Merged

GiudGiud mentioned this pull request Apr 5, 2022

Sibling (MultiApp to MultiApp) Transfers #19451

Closed

lindsayad reviewed Apr 15, 2022

View reviewed changes

lindsayad added a commit to lindsayad/moose that referenced this pull request Apr 15, 2022

Fix parameter getting in FEProblemBase::addTransfer

71a402a

Refs idaholab#19676

lindsayad mentioned this pull request Apr 15, 2022

Do not give cryptic error in FEProblemBase::addTransfer #20803

Merged

lindsayad added a commit to lindsayad/moose that referenced this pull request Apr 15, 2022

Do not give cryptic error in FEProblemBase::addTransfer

1ecd394

Also fix typos: 'directions' to 'direction' towards bottom of `MultiAppTransfer` constructor Refs idaholab#19676

lindsayad added a commit to lindsayad/moose that referenced this pull request Apr 15, 2022

Do not give cryptic error in FEProblemBase::addTransfer

946d8c4

Also fix typos: 'directions' to 'direction' towards bottom of `MultiAppTransfer` constructor Refs idaholab#19676

lindsayad added a commit to lindsayad/moose that referenced this pull request Apr 15, 2022

Do not give cryptic error in FEProblemBase::addTransfer

ec828e4

Also fix typos: 'directions' to 'direction' towards bottom of `MultiAppTransfer` constructor Refs idaholab#19676

lindsayad added a commit to lindsayad/moose that referenced this pull request Apr 19, 2022

Do not give cryptic error in FEProblemBase::addTransfer

4f98f85

Also fix typos: 'directions' to 'direction' towards bottom of `MultiAppTransfer` constructor Refs idaholab#19676

GiudGiud added a commit to idaholab/virtual_test_bed that referenced this pull request Apr 26, 2022

Adapt to MOOSE changes in multiapp parameters, refs idaholab/moose#19676

6799c49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add between multiapp (sibling) copy transfer #19676

Add between multiapp (sibling) copy transfer #19676

GiudGiud commented Dec 22, 2021 •

edited

moosebuild commented Dec 22, 2021 •

edited

moosebuild commented Dec 22, 2021 •

edited

loganharbour left a comment

fdkong commented Jan 6, 2022

fdkong commented Jan 6, 2022

fdkong commented Jan 6, 2022

GiudGiud commented Jan 14, 2022

GiudGiud commented Feb 11, 2022

GiudGiud commented Feb 18, 2022

moosebuild commented Mar 29, 2022

lindsayad Apr 15, 2022

lindsayad Apr 15, 2022

fdkong Apr 15, 2022

GiudGiud Apr 15, 2022

GiudGiud Apr 15, 2022

lindsayad Apr 15, 2022

permcody commented Oct 11, 2022 via email

Add between multiapp (sibling) copy transfer #19676

Add between multiapp (sibling) copy transfer #19676

Conversation

GiudGiud commented Dec 22, 2021 • edited

moosebuild commented Dec 22, 2021 • edited

moosebuild commented Dec 22, 2021 • edited

Framework coverage

Modules coverage

Functional expansion tools

Level set

Stochastic tools

Full coverage reports

Warnings

loganharbour left a comment

Choose a reason for hiding this comment

fdkong commented Jan 6, 2022

fdkong commented Jan 6, 2022

fdkong commented Jan 6, 2022

GiudGiud commented Jan 14, 2022

GiudGiud commented Feb 11, 2022

GiudGiud commented Feb 18, 2022

moosebuild commented Mar 29, 2022

lindsayad Apr 15, 2022

Choose a reason for hiding this comment

lindsayad Apr 15, 2022

Choose a reason for hiding this comment

fdkong Apr 15, 2022

Choose a reason for hiding this comment

GiudGiud Apr 15, 2022

Choose a reason for hiding this comment

GiudGiud Apr 15, 2022

Choose a reason for hiding this comment

lindsayad Apr 15, 2022

Choose a reason for hiding this comment

permcody commented Oct 11, 2022 via email

GiudGiud commented Dec 22, 2021 •

edited

moosebuild commented Dec 22, 2021 •

edited

moosebuild commented Dec 22, 2021 •

edited