Add override options for array context fallbacks

illinois-ceesd / mirgecom

MIRGE-Com is the workhorse simulation application for the Center for Exascale-Enabled Scramjet Design at the University of Illinois.

Other

11 stars 19 forks source link

Add override options for array context fallbacks #1007

Closed majosm closed 4 months ago

majosm commented 4 months ago

Adds options to initialize_actx reflecting the disabling of array context fallbacks and the override options that were added to meshmode/grudge. Also tweaked the implementation to try to simplify the logic a bit.

Questions for the review:

[ ] Is the scope and purpose of the PR clear?
- [ ] The PR should have a description.
- [ ] The PR should have a guide if needed (e.g., an ordering).
[ ] Is every top-level method and class documented? Are things that should be documented actually so?
[ ] Is the interface understandable? (I.e. can someone figure out what stuff does?) Is it well-defined?
[ ] Does the implementation do what the docstring claims?
[ ] Is everything that is implemented covered by tests?
[ ] Do you see any immediate risks or performance disadvantages with the design? Example: what do interface normals attach to?

matthiasdiener commented 4 months ago

LGTM, in general. Does this require the production branches of meshmode/grudge, or does it work with main too?

majosm commented 4 months ago

LGTM, in general. Does this require the production branches of meshmode/grudge, or does it work with main too?

I think it needs production of meshmode at least, since the fallback stuff is in FusionContractorArrayContext. On the grudge side though, it looks like the relevant code is in inducer/main (it references the fusion array context optionally, if it exists). So I guess potentially my changes could go into main of grudge instead of production?

matthiasdiener commented 4 months ago

So I guess potentially my changes could go into main of grudge instead of production?

Yeah, I think that would make sense. My concern was that when merging this PR, mirgecom would then not run anymore with the main branches of grudge/meshmode.

majosm commented 4 months ago

So I guess potentially my changes could go into main of grudge instead of production?

Yeah, I think that would make sense. My concern was that when merging this PR, mirgecom would then not run anymore with the main branches of grudge/meshmode.

I think mirgecom main already depends on production of grudge/meshmode for a couple of things (multivolume coupling and ESDG among them), so that ship may have already sailed. 🙂 But I'll still make a PR into grudge main.

MTCam commented 4 months ago

So I guess potentially my changes could go into main of grudge instead of production?

Yeah, I think that would make sense. My concern was that when merging this PR, mirgecom would then not run anymore with the main branches of grudge/meshmode.

I think mirgecom main already depends on production of grudge/meshmode for a couple of things (multivolume coupling and ESDG among them), so that ship may have already sailed. 🙂 But I'll still make a PR into grudge main.

I understand the concern, but I'm not sure it makes sense for us to delay the merge to mirgecom@main because it will cause mirgecom@main not to work with grudge@main. Now that the updates are already merged to ceesd/grudge@production and ceesd/meshmode@production, our prediction cases no longer work out of the box with the ceesd install. That's kind of a headache.

Is there any harm in merging this and making a PR against grudge@main that can be merged on the long timesscale that usually takes?

majosm commented 4 months ago

Is there any harm in merging this and making a PR against grudge@main that can be merged on the long timesscale that usually takes?

I think that would be OK. @matthiasdiener?

MTCam commented 4 months ago

Awesome. Thanks gents!