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

H(curl) basis and CEED_EVAL_CURL #1156

Merged
merged 8 commits into from
Apr 12, 2023

Conversation

sebastiangrimberg
Copy link
Collaborator

@sebastiangrimberg sebastiangrimberg commented Feb 15, 2023

Preliminary support for H(curl). Changes are very similar to #878.

This handles CeedBasis updates for H(curl) and also extends some of the missing H(div) functionality in ceed-preconditioning.c, for example. Any changes to element restriction for high-order Nedelec elements are left to a future PR, as is backend support (first non-tensor cuda/hip-ref, OCCA, and MAGMA, then later on cuda/hip-shared/gen).

Note: I opened this PR from a fork since I didn't have write access to CEED/libCEED, but if it's preferred to create the branch there then I can close and reopen a new PR once I have access.

TODO

  • Add tests similar to H(div) basis constructor #878.
  • Backend support: So far have only touched ref-basis, ref-operator, and blocked-operator. What do I need to do for CUDA/HIP/MAGMA/OCCA support for non-tensor H(div) and H(curl)? Maybe this is a bigger ask (Non-Tensor Basis Support #350).
    • Note: Left for a later PR
  • Diagonal assembly: Does anything special need to be done here? CeedOperatorGetBasisPointer? What about for p-multigrid?
    • Fixes to multigrid and operator assembly completed for CPU, left for later for backends
  • Update Julia/Python/Rust bindings. Also Fortran?
    • I updated the Julia bindings which seems to have added a few methods which I didn't touch (CeedOperatorContextGetDoubleRead, CeedOperatorContextRestoreDoubleRead, etc.). I think this might be just because the generator hadn't been run for a past PR, but let me know if I'm doing something incorrectly.

@jedbrown
Copy link
Member

Thanks! It looks like maybe your editor formatting configuration is different. Can you have your editor use clangd so the auto-formatter works, or use make format before committing to avoid whitespace noise?

@sebastiangrimberg
Copy link
Collaborator Author

Thanks! It looks like maybe your editor formatting configuration is different. Can you have your editor use clangd so the auto-formatter works, or use make format before committing to avoid whitespace noise?

I used make format but it might be an issue with my version of clang-format (Homebrew uses 15 now vs. Ubuntu 22 default is 14). I'll rerun tomorrow with v14!

@jeremylt
Copy link
Member

Our pipeline currently uses 14. I haven't had the chance to update yet

@jedbrown
Copy link
Member

Done.
#1157

include/ceed/ceed.h Outdated Show resolved Hide resolved
interface/ceed-basis.c Outdated Show resolved Hide resolved
interface/ceed-basis.c Outdated Show resolved Hide resolved
interface/ceed-operator.c Outdated Show resolved Hide resolved
Copy link
Member

@jeremylt jeremylt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re some of your points at the top

  1. I'm fine adding GPU support in a follow-up PR. That way this PR won't get too big. Adding initial [hip,cuda]/ref support won't be too bad, but [hip,cuda]/[shared,gen] is related to the other issue you pointed out

  2. Similarly, I think its good to add Diagonal assembly support in a follow-up PR. The multigrid infrastructure should be largely unaffected, though I'm not sure if our process for automatically building a coarse to fine interpolation works for H(div) and H(curl) elements?

  3. Python, Rust, and Julia bindings would be nice. Our Fortran interface is not a high priority but I wouldn't object to adding this there as well. Let me know if you get stuck trying to add any of those and I can help.

tests/t332-basis.c Outdated Show resolved Hide resolved
tests/t341-basis.c Outdated Show resolved Hide resolved
tests/t342-basis.c Outdated Show resolved Hide resolved
@sebastiangrimberg
Copy link
Collaborator Author

Re some of your points at the top

  1. I'm fine adding GPU support in a follow-up PR. That way this PR won't get too big. Adding initial [hip,cuda]/ref support won't be too bad, but [hip,cuda]/[shared,gen] is related to the other issue you pointed out

OK, sounds good. I am very unfamiliar with what will be required here, but I don't think it will be overly complex because H(div)/H(curl) interpolation contractions should look the same (or nearly) to H1 gradient.

  1. Similarly, I think its good to add Diagonal assembly support in a follow-up PR. The multigrid infrastructure should be largely unaffected, though I'm not sure if our process for automatically building a coarse to fine interpolation works for H(div) and H(curl) elements?

For multigrid, the issue I see is that CeedOperatorMultigridLevelCreate calls CeedBasisCreateProjection when prolongation or restriction operators are requested. This method I think needs fixing for H(div) and H(curl) interpolation.

  1. Python, Rust, and Julia bindings would be nice. Our Fortran interface is not a high priority but I wouldn't object to adding this there as well. Let me know if you get stuck trying to add any of those and I can help.

I see for Julia that there is an automated generator method in julia/libCEED.jl/gen which makes this easy. Is there something similar for Python and Rust or do I have to do it by hand?

@jeremylt
Copy link
Member

The raw bindings are automatic but the user facing bindings need to be hand-coded in all languages

@sebastiangrimberg
Copy link
Collaborator Author

Hi @jedbrown @jeremylt, I just pushed a few commits with a bit of a reworking of this idea. I realized that a general implementation for non-tensor H^1, H(div) and H(curl) basis operations should be possible with the benefit of simplifying backend code a lot. The general idea is that CEED_EVAL_INTERP in H(div) and H(curl) looks exactly like CEED_EVAL_GRAD for H^1, H(div) CEED_EVAL_DIV looks like H^1 CEED_EVAL_INTERP, and CEED_EVAL_CURL looks again looks like H^1 CEED_EVAL_GRAD(in 3D at least). So, I just tried to use the CeedBasisGetQuadratureNumComponents more extensively to make all of these operations look the same, with the output of CeedBasisApply being a q_comp x num_comp x num_qpts x num_elems tensor (where q_comp = 1 for H^1 interpolation, for example). For now you can check out the CPU implementation in CeedBasisApply_Ref and I'd be curious your thoughts.

In this way, the non-tensor cuda-ref and hip-ref backends should be very easy to port to H(div) and H(curl) with only some minor refactoring, and most of the methods in ceed-preconditioning.c should also require only a bit of modification to work with H(div) and H(curl).

@rezgarshakeri
Copy link
Collaborator

Hi @jedbrown @jeremylt, I just pushed a few commits with a bit of a reworking of this idea. I realized that a general implementation for non-tensor H^1, H(div) and H(curl) basis operations should be possible with the benefit of simplifying backend code a lot. The general idea is that CEED_EVAL_INTERP in H(div) and H(curl) looks exactly like CEED_EVAL_GRAD for H^1, H(div) CEED_EVAL_DIV looks like H^1 CEED_EVAL_INTERP, and CEED_EVAL_CURL looks again looks like H^1 CEED_EVAL_GRAD(in 3D at least). So, I just tried to use the CeedBasisGetQuadratureNumComponents more extensively to make all of these operations look the same, with the output of CeedBasisApply being a q_comp x num_comp x num_qpts x num_elems tensor (where q_comp = 1 for H^1 interpolation, for example). For now you can check out the CPU implementation in CeedBasisApply_Ref and I'd be curious your thoughts.

In this way, the non-tensor cuda-ref and hip-ref backends should be very easy to port to H(div) and H(curl) with only some minor refactoring, and most of the methods in ceed-preconditioning.c should also require only a bit of modification to work with H(div) and H(curl).

Not always. When the problem is scalar in H^1, yes CEED_EVAL_GRAD is similar to CEED_EVAL_INTERP in H(div).

@sebastiangrimberg
Copy link
Collaborator Author

sebastiangrimberg commented Feb 21, 2023

Not always. When the problem is scalar in H^1, yes CEED_EVAL_GRAD is similar to CEED_EVAL_INTERP in H(div).

Yep that's correct. My point was simply that with num_comp = 1 we should be able to reuse much of the H^1 gradient kernels (for example) and shouldn't need to write much code nor sacrifice performance.

@rezgarshakeri
Copy link
Collaborator

Not always. When the problem is scalar in H^1, yes CEED_EVAL_GRAD is similar to CEED_EVAL_INTERP in H(div).

Yep that's correct. My point was simply that with num_comp = 1 we should be able to reuse much of the H^1 gradient kernels (for example) and shouldn't need to write much code nor sacrifice performance.

Yeah, I got your point, I just wanted to mentioned that is true for scalar case and not using it otherwise.

include/ceed/backend.h Outdated Show resolved Hide resolved
include/ceed/ceed.h Outdated Show resolved Hide resolved
@sebastiangrimberg sebastiangrimberg force-pushed the sjg/hcurl-basis-dev branch 2 times, most recently from 63b1361 to 0c64355 Compare April 4, 2023 16:37
@sebastiangrimberg sebastiangrimberg force-pushed the sjg/hcurl-basis-dev branch 6 times, most recently from 89780b1 to 4a222ce Compare April 7, 2023 02:39
@jeremylt
Copy link
Member

jeremylt commented Apr 7, 2023

PR to update main to non-RELEASE merged into main

This refactors basis application to use common code between CEED_EVAL_GRAD for H^1 and CEED_EVAL_INTERP for H(curl)/H(div). It should ideally expose an easy way to get non-tensor backends to work for H(div) and H(curl).
For multigrid, H(curl) and H(div) projection bases don't have a CEED_EVAL_CURL or CEED_EVAL_DIV operation currently. I'm not sure if a similar formulation to CEED#1023 applies but for now it is probably OK without it.
Add Rust sibling functions and tests, add Python tests, fix Julia versioning for CI.
Copy link
Member

@jeremylt jeremylt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only thing that we need before merging is create_hdiv_basis and create_hcurl_basis tests for julia/LibCEED.jl/test/rundevtests.jl. Currently there are no dev tests, but there are some examples for create_h1_basis in ``julia/LibCEED.jl/test/runtests.jl`.

Everything else seems to be all set.

@sebastiangrimberg sebastiangrimberg force-pushed the sjg/hcurl-basis-dev branch 3 times, most recently from 91eae2f to 089e8c8 Compare April 10, 2023 18:32
@sebastiangrimberg
Copy link
Collaborator Author

sebastiangrimberg commented Apr 10, 2023

I think the only thing that we need before merging is create_hdiv_basis and create_hcurl_basis tests for julia/LibCEED.jl/test/rundevtests.jl. Currently there are no dev tests, but there are some examples for create_h1_basis in ``julia/LibCEED.jl/test/runtests.jl`.

Everything else seems to be all set.

Done! See b2e3f8e.

Copy link
Member

@jeremylt jeremylt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants