Rm unused member fn, if stmt, header includes, param registrations

[nmnobre]

Reason

The only thing still itching is {get,has}MFEMObject(), though not enough to hold this I don't think. Only one is templated but they both could be, and the template argument encloses sufficient information to determine the system unambiguously, right?

Originally posted by @nmnobre in #32701 (review)

Design

The key here is simply to retrieve the system attribute name from the template type:
const std::string system = T::validParams().getSystemAttributeName();

Impact

Devs won't have to deal with redundant system attribute names, simplifying the API calls.

EDIT: After much back and forth, we've decided to drop the initial purpose for this PR.

[moosebuild]

Job Documentation, step Docs: sync website on e4b0c1c wanted to post the following:

View the site here

This comment will be updated on new commits.

[moosebuild]

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

Framework coverage

	f7d8b0	#32781 e4b0c1
	Total	Total	+/-	New
Rate	85.90%	85.91%	+0.01%	100.00%
Hits	133158	133154	-4	16
Misses	21851	21838	-13	0

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

• framework
• chemical_reactions
• combined
• contact
• electromagnetics
• external_petsc_solver
• fluid_properties
• fsi
• functional_expansion_tools
• geochemistry
• heat_transfer
• level_set
• misc
• navier_stokes
• optimization
• peridynamics
• phase_field
• porous_flow
• ray_tracing
• rdg
• reactor
• scalar_transport
• solid_mechanics
• solid_properties
• stochastic_tools
• subchannel
• thermal_hydraulics
• xfem

This comment will be updated on new commits.

[moosebuild]

Job Precheck, step Clang format on 2bf8ed9 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/32781/clang_format/style.patch | git apply -v

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

git clang-format 0961ad13fbadf5ecc89198244e38a38d56bcedbd

[lindsayad]

Here are my thoughts:

• I think the warehouse change should be reverted. I like relying on filtering through attributes which have caches associated with them and then maintaining the invariant that queryInto should only be acting on candidates that match the type T
• I think all the system attribute name registrations should be restored. Those have a nice parallel with the libMesh backend
• I like the change to remove the type string parameter from getMFEMObject as it removes what feels like redundant typing at the call-sites. I wonder if we can restore the idea that was originally present in this PR with something like

  template <typename T>
  const std::string & mfemSystemName()
  {
    static const std::string system = []()
    {
      auto params = T::validParams();
      return params.getSystemAttributeName();
    }();
    return system;
  }

and then use in getMFEMObject like

  theWarehouse()
      .query()
      .condition<AttribSystem>(mfemSystemName<T>())
      .condition<AttribThread>(tid)
      .condition<AttribName>(name)
      .queryInto(objs);

Then we only pay the input parameters construction cost once per template instantiation

[nmnobre]

All done.

[lindsayad]

It's a little sad to slow down by that factor, but until someone complains about the overhead, I think the code cleanliness is worth it

[lindsayad]

An alternative that should bring back the .25%, while keeping the call-sites clean, is lindsayad/moose@8b46e400ee9.

[nmnobre]

An alternative that should bring back the .25%

What if we can get 0.20% instead? 😈
At the risk of pissing you off, here's this.

Btw, that metric is obtained with ./run_tests --dbg -C tests/mfem/timesteppers --verbose -t | grep MFEMProblem::getMFEMObject | awk -F'|' '{print $1, $6}' after adding TIME_SECTION("getMFEMObject", 1, "getMFEMObject").

[loganharbour]

here's this.

[lindsayad]

Lol. I spent too much time on this changing AttribSystem. The crazy refactor is now validated and is at lindsayad/moose@b87ecc305e8. These changes could be compatible even with the thing @loganharbour vomited at. But I suspect the timing with the refactor should be pretty good. Will try your timing command on Monday

[lindsayad]

typeid() would always compare false unless you're directly comparing at the level of the system registration. That's not the case with the implementation in my branch

[lindsayad]

I'm going to see whether we do that anywhere. I don't think we do.

I'm not seeing it anywhere

[nmnobre]

The derived class inherits the public data member.

You're right, my bad. I missed that. So it matches the semantics in this PR but avoids the performance hit.

an abuse of the original intent

Is it? We are currently doing it in next in hasMFEMObject(). And I actually think that's good. We save the performance hit of the dynamic cast in queryInto().

I'm sorry, but how bad is it if I say I actually prefer to keep things as they are in next? I don't really like all the boilerplate code we need to add... In addition to registering a base name (maybe still literally) and a system name (probably now retrieved), the dev now has to add a static member (which at first sight will look unintelligible as they need to know what it is for)... I think the literals are just more readable. From my side I'm happy to drop what I suggested, I'll just downgrade this PR to the other minor tweaks identified along the way. If what we take away from all this discussion is that the current solution in next is the best compromise, that's absolutely fine by me - this is what the scientific method is all about after all.

[lindsayad]

Is it? We are currently doing it in next in hasMFEMObject(). And I actually think that's good. We save the performance hit of the dynamic cast in queryInto().

Indeed, this is the case!

I'm sorry, but how bad is it if I say I actually prefer to keep things as they are in next? I don't really like all the boilerplate code we need to add... In addition to registering a base name (maybe still literally) and a system name (probably now retrieved), the dev now has to add a static member (which at first sight will look unintelligible as they need to know what it is for)... I think the literals are just more readable. From my side I'm happy to drop what I suggested, I'll just downgrade this PR to the other minor tweaks identified along the way. If what we take away from all this discussion is that the current solution in next is the best compromise, that's absolutely fine by me - this is what the scientific method is all about after all.

Haha it's not bad at all. I think that's a fine conclusion. It will save me days' work fixing apps missing includes of FEProblemBase.h 😄

[lindsayad]

Approving the new state of the PR

[moosebuild]

Job Test, step Results summary on e4b0c1c wanted to post the following:

Framework test summary

Compared against f7d8b00 in job civet.inl.gov/job/3796767.

No change

Modules test summary

Compared against f7d8b00 in job civet.inl.gov/job/3796767.

No change