Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Naming convention: volume/surface *integral* → *terms* #616

Closed
Tracked by #720
ranocha opened this issue May 27, 2021 · 11 comments
Closed
Tracked by #720

Naming convention: volume/surface *integral* → *terms* #616

ranocha opened this issue May 27, 2021 · 11 comments
Labels
breaking discussion triage To be decided at the next Trixi meeting

Comments

@ranocha
Copy link
Member

ranocha commented May 27, 2021

I started to think about implementing FD methods today. I will use the DG struct for that, since block SBP FD methods are just DG methods with another basis (and another structure of the derivative operator, changing the way to efficient implementations completely). Since FD methods are usually implemented in strong form, the term integral for volume/surface contributions doesn't really make sense anymore, I think. Something like volume/surface terms seems to be more appropriate and general, I think.

We should discuss this. If we agree to make the change, we should add appropriate deprecations, mention them in the NEWS.md, and remove the deprecated stuff in Trixi v0.4.

@sloede
Copy link
Member

sloede commented May 27, 2021

Could you give a few examples regarding what would change and how?

@ranocha
Copy link
Member Author

ranocha commented May 27, 2021

  • VolumeIntegralWeakFormVolumeTermsWeakForm etc. These are exported/documented and need to be deprecated.
  • calc_volume_integral!calc_volume_terms! or just volume_terms! etc. These are not exported/documented, so we don't need to deprecate them.

@gregorgassner
Copy link
Contributor

In principle, DG is based on integrals and hence one could argue calling it VolumeIntegral or SurfaceIntegral makes sense.
SBP FD could/should have different names, I agree.

Is it necessary, that DG and SBP have the same names? We could have different names for different solvers if you ask me.
Why do you want to unify the naming?

@ranocha
Copy link
Member Author

ranocha commented May 27, 2021

I am strongly against using different names for the same abstraction depending on the context. That totally defeats multiple dispatch and code reuse.
As an extreme example, your proposal would mean to write dgemm for matrix multiplication of real elements, zgemm for complex matrices etc. I prefer to write * (or the in-place version mul!) for all of them.

@ranocha
Copy link
Member Author

ranocha commented May 27, 2021

Additionally, I don't think DGSEM people will be confused by VolumeTerms or SurfaceTerms. In contrast, FD people will have no clue what kind of integral you're talking about. Why shouldn't we pick the more general name that's easier to understand for a broader audience?
In addition, we don't see the integrals anymore in the implementations of DGSEM... or SBP on triangles...

@ranocha
Copy link
Member Author

ranocha commented May 27, 2021

(I'm using some rather extreme wording here to play the devil's advocate. I knew that we will have such discussions and we definitely need them to find a good solution for all of us and Trixi. That's why I chose the tag discussion for this issue.)

@jlchan
Copy link
Contributor

jlchan commented May 27, 2021

I don't think most of these code changes affect me, but FWIW, I've also used VolumeTerms and SurfaceTerms before too (the "strong form" DG implementation on triangles/tets is further removed from an integral compared with DGSEM).

I also think the "volume integral" for entropy stable DGSEM looks pretty different from a standard volume integral...

@sloede
Copy link
Member

sloede commented May 27, 2021

Why shouldn't we pick the more general name that's easier to understand for a broader audience?

For similar reasons why we have separate implementations for 1D/2D/3D: If you are omniscient and/or have multiple years of high-order numerical analysis experience under your belt, all naming conventions are more or less OK for you. However, especially for new users it can be easier to only see a particular name/implementation that is specific to the community you're coming from. For instance, volume integral and surface integral are terms that are immediately recognizable for anyone growing up on David Kopriva's blue book. "Volume terms" and "surface terms" might also refer to other details, or just part of the integrals etc.

I am not saying that we shouldn't do it, I am just saying what's "easier to understand" can usually not be determined in an absolute sense, but depends heavily on the perspective...

@ranocha
Copy link
Member Author

ranocha commented May 27, 2021

For instance, volume integral and surface integral are terms that are immediately recognizable for anyone growing up on David Kopriva's blue book.

Well, I didn't grow up that way. I'm more an autodidact and never had any formal lecture about DG methods or numerical methods for hyperbolic PDEs. Personally, I see DGSEM as "just" a special case of SBP methods.

But you're right, there are reasons to keep the current name. On the other hand, there are also reasons to change it. What I would really like to avoid is having the same abstraction multiple times but doing the same thing.

@ranocha ranocha added the triage To be decided at the next Trixi meeting label Jun 1, 2021
@sloede
Copy link
Member

sloede commented Jun 3, 2021

If it is the consensus to rename it to volume_terms/surface_terms, I can live with it as well. It would be great to have a comment somewhere - maybe in the respective docstrings of the DGSEM and FDSBP types - explaining what the terms volume_terms/surface_terms means in this context? I know I tend to ask for a lot of commentary, but I feel like this makes it much easier for new users to understand the code, especially if they are still learning the underlying methods as well (= students).

In any case, I would propose to do this in a different PR than #617 to make it not a bigger PR than necessary.

@ranocha ranocha mentioned this issue Jul 13, 2021
36 tasks
@ranocha
Copy link
Member Author

ranocha commented Oct 6, 2021

Decided in our meeting some time ago: Nobody has a very strong opinion on this. Thus, it would mostly be an "annoying" change in the sense that it doesn't provide new features while requiring code updates. Thus, we postpone possible changes of the naming convention until they are pressing matters.

@ranocha ranocha closed this as completed Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking discussion triage To be decided at the next Trixi meeting
Projects
None yet
Development

No branches or pull requests

4 participants