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

Add generic nodal/modal bilinear form routines #103

Merged
merged 33 commits into from
Feb 5, 2025

Conversation

a-alveyblanc
Copy link
Contributor

@a-alveyblanc a-alveyblanc commented Dec 5, 2024

It would be helpful to have a more general version of nodal_quad_mass_matrix. My specific use case for this routine is generating operators representing bilinear forms that have derivatives on the test functions but not necessarily on the trial functions. More generally, this can be useful to generate operators representing bilinear forms whose test and trial functions are not the same.

Functionality of nodal_quad_mass_matrix and modal_quad_mass_matrix are updated to use the new, general routines.

@a-alveyblanc
Copy link
Contributor Author

Not sure why ruff was failing here. It wasn't failing locally, and I didn't change any of the files the ruff CI test was complaining about. Fixed them anyway, but happy to revert if it seems like a CI bug.

The changes in contrib/weights-from-festa-sommariva were the ones needed to make ruff happy.

@alexfikl
Copy link
Collaborator

alexfikl commented Dec 6, 2024

Not sure why ruff was failing here. It wasn't failing locally, and I didn't change any of the files the ruff CI test was complaining about. Fixed them anyway, but happy to revert if it seems like a CI bug.

The changes in contrib/weights-from-festa-sommariva were the ones needed to make ruff happy.

That's a new check in ruff 0.8.2: astral-sh/ruff#14611. Thanks for fixing it!

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! Just a few style quibbles below.

@a-alveyblanc
Copy link
Contributor Author

Seems like mypy also now has issue with something I didn't touch. I'll take a closer look later today.

@alexfikl
Copy link
Collaborator

Seems like mypy also now has issue with something I didn't touch. I'll take a closer look later today.

Those seem to be due to an issue with some new numpy typing changes: numpy/numpy#27957.

@a-alveyblanc
Copy link
Contributor Author

a-alveyblanc commented Dec 14, 2024

The first go was a confusing amalgamation of two versions of applying an operator to evaluate a bilinear form. They should now be cleanly separated with names that do a better job of describing what's happening.

What I mean by "two versions" is:

  1. We can first compute a "half" operator, i.e. the test functions evaluated at quadrature nodes times quadrature weights, then apply that to a trial solution evaluated at quadrature nodes (implemented as modal_quadrature_operator and nodal_quadrature_operator), or
  2. We can first compute a "full" operator, i.e. compute the inner product of test and trial functions using quadrature, then apply that to the coefficients of the trial solution at discretization nodes (implemented as modal_quadrature_bilinear_form and nodal_quadrature_bilinear_form)

Hopefully this is more clear. Happy to convert to a draft if we think more work needs to be done

Copy link
Collaborator

@alexfikl alexfikl left a comment

Choose a reason for hiding this comment

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

Just took a quick look and left some comments 😁

@inducer
Copy link
Owner

inducer commented Dec 17, 2024

Unsubscribing... @-mention or request review once it's ready for a look or needs attention.

@a-alveyblanc
Copy link
Contributor Author

@inducer This is ready for another look

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Some thoughts from a quick scroll below.

@inducer inducer merged commit 8ba8754 into inducer:main Feb 5, 2025
9 checks passed
@inducer
Copy link
Owner

inducer commented Feb 5, 2025

Thanks, 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.

3 participants