moose icon indicating copy to clipboard operation
moose copied to clipboard

General multiapp field transfer

Open fdkong opened this issue 3 years ago • 9 comments

Created a new transfer

Working on these features

  1. Block restricted transfers #16427.

  2. MultiApp transfers support array and vector variables #16429

fdkong avatar Mar 23 '21 19:03 fdkong

Job Documentation on 3d613e2 wanted to post the following:

View the site here

This comment will be updated on new commits.

moosebuild avatar Mar 23 '21 21:03 moosebuild

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jun 26 '21 03:06 stale[bot]

Nope

fdkong avatar Jun 26 '21 13:06 fdkong

Check where we are

fdkong avatar Mar 01 '22 21:03 fdkong

Job Coverage on 3d613e2 wanted to post the following:

Framework coverage

ef49eb #17417 3d613e
Total Total +/- New
Rate 84.62% 84.70% +0.08% 89.40%
Hits 81980 82830 +850 961
Misses 14896 14963 +67 114

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 89.40% is less than the suggested 90.0%

This comment will be updated on new commits.

moosebuild avatar Mar 02 '22 02:03 moosebuild

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

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

git clang-format 4ab280bda315b1c517a87a11c3f0742caab75668

moosebuild avatar Mar 16 '22 23:03 moosebuild

Is it possible to replace the existing mesh function and nearest node transfers with the new versions here?

lindsayad avatar Jul 25 '22 19:07 lindsayad

Is it possible to replace the existing mesh function and nearest node transfers with the new versions here?

That is the plan. I can not see why not. But need to make sure the replacement happen gradually. It is not a good idea to remove old ones while new ones have not been widely utilized yet

@GiudGiud @lindsayad should help me get the PR in, lol

fdkong avatar Jul 25 '22 19:07 fdkong

Is it possible to replace the existing mesh function and nearest node transfers with the new versions here?

That is the plan. I can not see why not. But need to make sure the replacement happen gradually. It is not a good idea to remove old ones while new ones have not been widely utilized yet

I agree with that. I m going to start working on getting this in next week. This looks awesome already, just needs some finishing touches.

GiudGiud avatar Sep 21 '22 20:09 GiudGiud

Job Precheck on 3c009f6 wanted to post the following:

Warning: This PR changes repo size by 8.66 MiB.

moosebuild avatar Dec 03 '22 19:12 moosebuild

All jobs on 20d5388 : invalidated by @GiudGiud

if civet isnt upset about working during curtailment this will be green

moosebuild avatar Dec 29 '22 23:12 moosebuild

Coverage wise the only shocking thing is the lack of coverage on the overlap/value conflict/floating point indetermination search in parallel. This is actually likely just because the recipe doesn't cover all the parallel recipes, as it's explicitly targeted by a test

GiudGiud avatar Jan 01 '23 01:01 GiudGiud

Some profiles with tests at higher refinement.

Serial UO_3d.pdf NN_3d.pdf UO_2d.pdf UO_BR_2d.pdf (most interesting) UO - 3D test with r5 : 16,538,562 dofs UO - 2D test with r6 : 1,640,962 dofs UO - 2D test with r7 + block restriction + higher order variables : 5M dofs NN - 3D test with r4 : 2,086,882 dofs

For UO transfer we see the point locator creation and that is it. We could avoid creating them when they are not needed (no block or boundary restriction OR NN transfer)? Doesn't seem like a huge gain. The slow parts are initial conditions, the BoundaryNodeIntegrityCheckThread and the mesh refinement in 3D For block restriction : not a big cost overall For higher order variable: quite a cost from the generic projector, seems to be a bunch of mallocs and appends. This should be done on every iteration though, there's no way around it. Maybe if there's a way to update the projector at a lower cost, not sure. I would need to build libmesh from source to investigate at this point. The append that show up in the profile could be vector push_back getting expensive if the vectors are not sized correctly, but looking at the routine there's only one.

Maybe with more timesteps we will start seeing the transfers in profiling I think we can wait until we get feedback on this from users

Parallel WIP I ll update when I get back, maybe

GiudGiud avatar Jan 02 '23 14:01 GiudGiud

There's definitely a lot of short vector use in GenericProjector that I'd think could get a lot faster from a Small-Size Optimization pass ... but IIRC when we tried that for the vectors in AD we didn't see any speedup at all, so my intuition isn't to be trusted here.

roystgnr avatar Jan 04 '23 19:01 roystgnr

Sounds good. The failure with Griffin is trivial btw, this is ready for review

GiudGiud avatar Jan 04 '23 23:01 GiudGiud

Score! Those Distributed mesh sweep issues look unrelated too ... though I'm a bit concerned about what might have caused them to regress.

I'll start going through the code tomorrow.

roystgnr avatar Jan 05 '23 03:01 roystgnr

The strings-for-indexing comments from @friedmud are still applicable issues?

I think the various profiles I posted show these are negligible

GiudGiud avatar Jan 06 '23 12:01 GiudGiud

@roystgnr @loganharbour the distributed mesh failures here are not tied to this PR. They are tied to me turning on possibly an old distributed mesh sweep recipe. The current one on next does 2,4,8,16. https://civet.inl.gov/job/1286241/ The one that is failing here is 2,4,6,8,10,12,14,16. https://civet.inl.gov/job/1279751/ 10 and 12 are failing on tests unrelated to my PR.

I ll try to fix them but maybe not on this PR

GiudGiud avatar Jan 14 '23 12:01 GiudGiud

Get ready, last time I got 900 emails about the size of the repo

GiudGiud avatar Jan 14 '23 12:01 GiudGiud

It seems with those absurd build errors I missed that the nearest_app tests are failing at some ranks. I ll try to find time to look at them.

GiudGiud avatar Jan 22 '23 06:01 GiudGiud

We cant avoid perturbed lookups. This comes from the multiplicity of target sub app positions.

I think a parameter to the map would need to be part of the template for it since the maps are not class members of General Field Transfer. I ll see what I can do. I might just forbid this transfer for small meshes or force an error for near 0 insertions.

GiudGiud avatar Jan 25 '23 21:01 GiudGiud

Forbidding the transfer for (very) small meshes might be reasonable. Forcing an error for near-0 insertions, not so reasonable; it's too easy to do some mesh manipulation that you expect to leave a vertex at 0 but then run into FP error and end up with a vertex at 3e-12 or so instead.

roystgnr avatar Jan 25 '23 21:01 roystgnr

since we are only doing a (=0) + b for insertion then (a+b) - b for retrieval I think we are guaranteed that at exactly 0 we will never have floating point errors? Is b - b = 0 guaranteed by floating point math?

now if you built your mesh at 1e-13 because your mesher output 1e-13 instead of 0 for the first point, then we're out of luck.

GiudGiud avatar Jan 25 '23 22:01 GiudGiud

but we can just error then. And then whoever wants that will have to offset their mesh by (1, 1, 1) to make it work.

GiudGiud avatar Jan 25 '23 22:01 GiudGiud

I think I have a lead on setting a dimension on the map. I think it s possible now, thanks to the struct around the map

GiudGiud avatar Jan 25 '23 23:01 GiudGiud

now if you built your mesh at 1e-13 because your mesher output 1e-13 instead of 0 for the first point, then we're out of luck.

This literally is one use case I'm thinking of. I discovered while bug hunting at one point that Cubit was introducing epsilon FP error even to what ought to have been a very simple cartesian grid.

But there are any number of other cases that will end up there. Not long ago someone tried to do a 180 degree rotation and expected a precise result and got thwarted by the fact that sin(double(pi)) is not zero.

roystgnr avatar Jan 25 '23 23:01 roystgnr

@roystgnr I worked around the issue. See the commit message for the explanation on the design

GiudGiud avatar Jan 30 '23 16:01 GiudGiud

Job Precheck on 3d613e2 wanted to post the following:

Warning: This PR changes repo size by 14.43 MiB.

moosebuild avatar Jan 30 '23 22:01 moosebuild

Yeah, that's an approval. I think we're good to merge once whatever's going on with Griffin gets sorted.

roystgnr avatar Jan 31 '23 17:01 roystgnr

wouhou!

Thanks for the careful reviews @roystgnr and thanks for the excellent work @fdkong !! It was a pleasure building on it

GiudGiud avatar Feb 02 '23 18:02 GiudGiud

Thanks @GiudGiud . You have all credits!

Great work.

fdkong avatar Feb 02 '23 18:02 fdkong

Thanks to all of you ;-)

YaqiWang avatar Feb 02 '23 19:02 YaqiWang