replace nodal to modal transformation with an element-wise integral

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

replace nodal to modal transformation with an element-wise integral #1035

Closed anderson2981 closed 2 months ago

anderson2981 commented 2 months ago

Questions for the review:

[x] 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).
[x] Is every top-level method and class documented? Are things that should be documented actually so?
[x] Is the interface understandable? (I.e. can someone figure out what stuff does?) Is it well-defined?
[x] Does the implementation do what the docstring claims?
[x] Is everything that is implemented covered by tests?
[x] Do you see any immediate risks or performance disadvantages with the design? Example: what do interface normals attach to?

tulioricci commented 2 months ago

Is it improving performance? I can't remember exactly but I used the modal implementation because it was better (at the time) then using what I believe to be the built-in average.

anderson2981 commented 2 months ago

Is it improving performance? I can't remember exactly but I used the modal implementation because it was better (at the time) then using what I believe to be the built-in average.

It appears to, yes. The modal to nodal seems to invoke a large compile time penalty. This is part of the reason that we broke the filtering out of the main compiled rhs function. I haven’t tested performance yet for simplicies. For my case with TPE quads, the compile time went from 4000 s to 1000 s on my mac. The time per timestep went from 2.1 to 1.1 s.

anderson2981 commented 2 months ago

Any chance you could add your new limiter tests before we merge?

They should be live in y3prediction@main.