Skip to content

Conversation

pitkajuh
Copy link
Contributor

@pitkajuh pitkajuh commented Apr 1, 2024

This Pull request:

Changes or fixes:

Dear All. This pull request adds gradient and Laplacian methods for arrays. These can be used to calculate one-dimensional first and second order derivatives. Should there also be methods for calculating higher order derivatives and dimensions?

I am open to any feedback regarding the code.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #14304

@phsft-bot
Copy link

Can one of the admins verify this patch?

Copy link
Collaborator

@ferdymercury ferdymercury left a comment

Choose a reason for hiding this comment

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

See my suggestions for Gradient. Consider applying the same kind of changes to Laplacian. Thanks!

Copy link
Collaborator

@ferdymercury ferdymercury left a comment

Choose a reason for hiding this comment

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

Sorry, I edited now, crash should be gone

Copy link
Contributor Author

@pitkajuh pitkajuh left a comment

Choose a reason for hiding this comment

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

Changes ok.

@ferdymercury
Copy link
Collaborator

Changes ok.

Thanks! From my side, the only thing left is improving the documentation of Laplacian, try copy-pasting from Gradient and adapting.

@pitkajuh pitkajuh force-pushed the fix-14304 branch 2 times, most recently from c4a28aa to 689204e Compare April 14, 2024 13:17
@pitkajuh
Copy link
Contributor Author

Changes ok.

Thanks! From my side, the only thing left is improving the documentation of Laplacian, try copy-pasting from Gradient and adapting.

Thank you for providing suggestions and taking your time to review the code.

@pitkajuh pitkajuh marked this pull request as ready for review April 14, 2024 13:20
@pitkajuh pitkajuh requested a review from lmoneta as a code owner April 14, 2024 13:20
Copy link
Collaborator

@ferdymercury ferdymercury left a comment

Choose a reason for hiding this comment

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

You are welcome. Min N must be changed for Laplacian. Thanks!

@pitkajuh
Copy link
Contributor Author

You are welcome. Min N must be changed for Laplacian. Thanks!

Done.

Copy link

github-actions bot commented Apr 14, 2024

Test Results

     9 files       9 suites   1d 16h 55m 38s ⏱️
 2 635 tests  2 635 ✅ 0 💤 0 ❌
22 356 runs  22 356 ✅ 0 💤 0 ❌

Results for commit 61618a1.

♻️ This comment has been updated with latest results.

@ferdymercury
Copy link
Collaborator

Copy link
Collaborator

@ferdymercury ferdymercury left a comment

Choose a reason for hiding this comment

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

Why the change in roofitcore and bindings?

Also go to "Files changed" tab and revise the comments on testTMath.cxx, they are marked as resolved by mistake.

Thanks a lot!

@pitkajuh pitkajuh force-pushed the fix-14304 branch 5 times, most recently from 43a3658 to 519bdc9 Compare May 10, 2024 12:15
@pitkajuh
Copy link
Contributor Author

Why the change in roofitcore and bindings?

Also go to "Files changed" tab and revise the comments on testTMath.cxx, they are marked as resolved by mistake.

Thanks a lot!

Thank you for commenting. I am not sure what happened, but I think that the changes occured accidentally when merging from updated master branch. Those changes have been removed and I accepted the changes to the to testTMath.cxx.

I noticed that there is one suggestion on TMath.h (https://github.com/root-project/root/pull/15100/files#r1563955528) and I am not whether it has been implemented to the code. I am having an issue trying to resolve this suggestion. For some reason it does not disappear from the files changed tab, and after trying to accept suggestion multiple times, it get added to the code multiple times. So I ended up with something like this

template <typename T>
T *TMath::Gradient(Long64_t n, T *f, double h)
{
   if (!f) {
      ::Error("TMath::Gradient", "Input parameter f is empty.");
      return nullptr;
   } else if (n < 2) {
      ::Error("TMath::Gradient", "Input parameter n=%lld is smaller than 2.", n);
      return nullptr;
   }
  if (!f) {
      ::Error("TMath::Gradient", "Input parameter f is empty.");
      return nullptr;
   } else if (n < 2) {
      ::Error("TMath::Gradient", "Input parameter n=%lld is smaller than 2.", n);
      return nullptr;
   }
  if (!f) {
      ::Error("TMath::Gradient", "Input parameter f is empty.");
      return nullptr;
   } else if (n < 2) {
      ::Error("TMath::Gradient", "Input parameter n=%lld is smaller than 2.", n);
      return nullptr;
   }
  if (!f) {
      ::Error("TMath::Gradient", "Input parameter f is empty.");
      return nullptr;
   } else if (n < 2) {
      ::Error("TMath::Gradient", "Input parameter n=%lld is smaller than 2.", n);
      return nullptr;
   }
   Long64_t i = 1;

I removed the redundant if statements and the suggestion still shows up in the files changed tab. I am not sure what to do.

@ferdymercury
Copy link
Collaborator

That might be just a 'browser cache' issue. Refresh your window, now it looks correct to me.

@guitargeek guitargeek assigned guitargeek and unassigned pitkajuh May 20, 2024
Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you for this new addition in TMath

@dpiparo dpiparo merged commit 790fdfa into root-project:master Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[math] Finite difference methods for Gradient
6 participants