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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interpolation in Parthenon #1015

Closed
pgrete opened this issue Mar 7, 2024 · 4 comments 路 Fixed by #1020
Closed

Interpolation in Parthenon #1015

pgrete opened this issue Mar 7, 2024 · 4 comments 路 Fixed by #1020
Labels
question Further information is requested

Comments

@pgrete
Copy link
Collaborator

pgrete commented Mar 7, 2024

New day, new question 馃槂

I happily derived tracers for AthenaPK from @AstroBarker implementations in Phoebus (note this is the first non-default "package" in AthenaPK) and I'm now wondering what to do about interpolation.
I saw that the ones in Phoebus carry a line of dependencies: singularity-eos -> spiner -> ports-of-call

Are the any plans to upstream (some simplified versions) or would that not be a good idea, @brryan @Yurlungur ?
If I followed the details correctly, a minimum version would not require that much code additional code in comparison to what's already in Phoebus.

@pgrete pgrete added the question Further information is requested label Mar 7, 2024
@AstroBarker
Copy link

Glad to hear it was useful :) Regarding whether or not it is the right move to upstream, I defer to others, but I think it should be straightforward to do. The phoebus tracer interpolation is just tri/bi/linear interpolation here. The only thing used from spiner in this particular case is the weights_t struct.

@Yurlungur
Copy link
Collaborator

As @AstroBarker said, we don't really use singularity-eos in the interpolation routines. I'd be remiss to depend on singularity in parthenon as it's physics and might interfere with our open source permissions on the LANL end. Regarding the rest of the dependency chain:

  • spiner is a tiny table interpolation library I wrote. It re-implements something similar to our ParArrayNDs but tied to interpolation routines as well. We use it for tabulated data in singularity as well as phoebus. In the tracer machinery, we pulled some of the routines from it for convenience.
  • ports-of-call was originally, for historical and political reasons, a tool for "turning off" Kokkos and instead doing our own serial or Cuda routines. It can still do that, but it has grown a bit to provide, for example, error handling routines very similar to the ones in Parthenon.

I'd be happy to upstream our interpolation tooling. There's two paths to that:

  1. Just duplicate the relevant spiner things. This is simple, no new dependencies. But if we pull in more of spiner over time, we might eventually end up duplicating the whole library.
  2. Depending on spiner. We use submodules for that dependency chain in Phoebus, and parthenon could support the same. And spiner is very lightweight. That said, it would be an additional dependency.

@pgrete
Copy link
Collaborator Author

pgrete commented Mar 12, 2024

I now put everything into one file, see https://github.com/parthenon-hpc-lab/athenapk/blob/pgrete/tracer/src/utils/interpolation.hpp and also "reused" the robust pieces (which is a cool idea!).

In general, I think that this kind of functionality would be great to be available from within Parthenon either directly (i.e., moving the merged file upstream) or as an (optional) submodule.
For the latter, my only wish is that there are no deeper nested dependencies (like ports-of-call) in order to keep things simple.

@Yurlungur
Copy link
Collaborator

In general, I think that this kind of functionality would be great to be available from within Parthenon either directly (i.e., moving the merged file upstream) or as an (optional) submodule.

I'm super in favor of this being upstreamed. Since you already have it ported over for athenapk do you want to just submit a PR? I can also find some time to do that if you prefer.

@pgrete pgrete linked a pull request Mar 13, 2024 that will close this issue
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants