MultiFabRegister: throw in get

[ax3l]

Close #5319
Follow-up to #5230

• Throw a runtime exception instead of returning a nullptr if a field is requested via the getter.
• update logic to ensure this passes all tests

[EZoni]

Discussed with @RemiLehe offline:

The functions get_mr_levels and get_mr_levels_alldirs fails by definition when we call it on coarse-patch MultiFabs, e.g., current_cp, because those are allocated only on levels greater than level 0 while the function attempts to return all levels starting from 0.

We will try to add to get_mr_levels and get_mr_levels_alldirs a function argument, e.g., skip_level_0, such that the function returns a nullptr for level 0 instead of calling the internal functions of the MultiFabRegister and throwing an exception.

[EZoni]

@ax3l @RemiLehe

Just a heads-up, I rebased and pushed a few more fixes and this might be almost ready. Let's see the latest CI status after the last commit and move from there.

[EZoni]

This is the only CI failure left after fb0dc3c:

The following tests FAILED:
	417 - test_3d_effective_potential_electrostatic_picmi.run (Failed)
	418 - test_3d_effective_potential_electrostatic_picmi.analysis (Failed)
	419 - test_3d_effective_potential_electrostatic_picmi.checksum (Failed)

I pushed 57e8e22 to try to fix the test.

[EZoni]

@ax3l @lucafedeli88 @RemiLehe

CI is green now and the PR is ready for review.

[ax3l]

Excellent! Just a small Doxygen update needed and a few ideas for later improvements.

[ax3l]

hehe, that's one way to quickly get the behavior in without changing downstream :D okok, nice idea and we can make this more robust later.

[ax3l]

Is pushing back a nullptr the clean approach or not pushing back anything (skip might imply either).
Then on the other hand, I assume if we move the level indexing on skip that can cause a lot of confusion, too (one argument to not use a Vector for the list of pointers.)

Thus, ok for me to keep as you suggest for now.

[EZoni]

@ax3l

Thanks for reviewing my changes.

Yes, in some corner cases (e.g., F in the PML) I just pushed the issue to the next "layer". Those were changes that date back to last year, I might have been unsure about how to resolve those at the time.

I'll approve on my end and let @roelof-groenewald or @lucafedeli88 chime in further.

I would make sure that people have enough time to look at this PR and double check the changes, since they are not trivial.

[roelof-groenewald]

The changes are fine, but I wonder if we can rename the skip_level_0 variable to something else, maybe skip_level_0_for_coarse_patch. The reason I say that is if someone reads the code in the future, seeing just skip_level_0 = true, in something like ComputeSpaceChargeField() it could seem like the potential is not calculated for level 0 at all. Does that make sense?

[EZoni]

Thanks, @roelof-groenewald. I renamed the variable to skip_lev0_coarse_patch, just to shorten the name as much as possible without missing any crucial component.

[ax3l]

I think renaming in the usage "usage" code constants to skip_lev0_coarse_patch made sense.

In the MultiFabRegister class it does not make sense to me: this flag works for any MF (any patch, fine or coarse), if you pass it level 0 is skipped. I would thus suggest to keep skip_level_0 as the argument name in MultiFabRegister.H/.cpp if that makes sense @EZoni @roelof-groenewald

(Alternatively, do not add the new arg to the API at all and instead in user-code zero-out the 0-level coarse patch component on your skip_lev0_coarse_patch variable or even better, pass skip_lev0_coarse_patch to the call that should not process the level to be explicit. I am not sure how universally useful this argument addition in MultiFabRegister truly is, looks a bit hacky to me.)

[EZoni]

Thanks, @ax3l.

My bad, I went through the renaming too fast. I agree that it would make sense to use two different names.

As for this,

(Alternatively, do not add the new arg to the API at all and instead in user-code zero-out the 0-level coarse patch component on your skip_lev0_coarse_patch variable or even better, pass skip_lev0_coarse_patch to the call that should not process the level to be explicit. I am not sure how universally useful this argument addition in MultiFabRegister truly is, looks a bit hacky to me.)

I think I don't understand the suggestion. The reason for introducing the variable was that we have two methods, get_mr_levels and get_mr_levels_alldirs, that loop over all MR levels, and therefore we cannot call those methods on any coarse-patch MultiFab, e.g., rho_cp, because nothing is allocated for those MultiFabs on level 0. I think what I don't understand in the suggestion is what data we would "zero out": my understanding is that there would be no data to zero out because the underlying allocation is missing, since coarse-patch data is not allocated on level 0. I think this was @RemiLehe's and my understanding when we discussed what is summarized in #5356 (comment) and came up with the skip_level_0 workaround.

[ax3l]

Perfect.

Per last suggestion: oh yes, you are absolutely right that would not work 😂 ok, yes since this applies actually to all our cp, let's have it in the API (with the name back to the original).