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

Adding biharmonic energy function #84

Merged
merged 9 commits into from
Oct 19, 2023
Merged

Adding biharmonic energy function #84

merged 9 commits into from
Oct 19, 2023

Conversation

odedstein
Copy link
Collaborator

Adding a function to implement the biharmonic energy, both intrinsic and extrinsic.

Also adding grad_intrinsic, following the gptoolbox example.

@odedstein odedstein requested a review from sgsellan August 15, 2023 22:36
@odedstein odedstein self-assigned this Aug 15, 2023
@sgsellan
Copy link
Owner

Thanks! Some questions before merging:

  1. Could you improve the first docstring line in "biharmonic energy"? Specially since this is not actually computing an energy, rather, building a matrix. Maybe something like "Returns a matrix Q such that if u is a vector of vertex samples of a function f on the mesh V,F, then u'Qu is an approximation of the integral \int (laplacian (f))^2 (if energy='...') or \int ||Hessian(f)|| (if ...) ...

  2. We usually have the convention of starting with underscore functions that are only used internally in some other function. Does it make sense to change hessian_energy to _hessian_energy?

  3. Could you double-check the docstring in biharmonic_energy_intrinsic? There may be some copy-paste issues I think.

  4. I pushed minor changes making it consinstent that we add in the "Notes" where we got the function from.

  5. What is up with test_single_triangle_2d in the test_biharmonic_energy script?

@sgsellan
Copy link
Owner

^ if you feel like you've addressed those, feel free to merge. Everything else looks fine

@odedstein
Copy link
Collaborator Author

Thanks! Some questions before merging:

  1. Could you improve the first docstring line in "biharmonic energy"? Specially since this is not actually computing an energy, rather, building a matrix. Maybe something like "Returns a matrix Q such that if u is a vector of vertex samples of a function f on the mesh V,F, then u'_Q_u is an approximation of the integral \int (laplacian (f))^2 (if energy='...') or \int ||Hessian(f)|| (if ...) ...
  2. We usually have the convention of starting with underscore functions that are only used internally in some other function. Does it make sense to change hessian_energy to _hessian_energy?
  3. Could you double-check the docstring in biharmonic_energy_intrinsic? There may be some copy-paste issues I think.
  4. I pushed minor changes making it consinstent that we add in the "Notes" where we got the function from.
  5. What is up with test_single_triangle_2d in the test_biharmonic_energy script?

I addressed all of these. Will merge if the checks pass.

@odedstein odedstein merged commit c71b42f into main Oct 19, 2023
27 checks passed
@sgsellan sgsellan deleted the biharmonic_energy branch October 25, 2023 15:38
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.

2 participants