root icon indicating copy to clipboard operation
root copied to clipboard

[TMath] Add Gradient and Laplacian methods for arrays

Open pitkajuh opened this issue 1 year ago • 8 comments

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:

  • [x] tested changes locally
  • [x] updated the docs (if necessary)

This PR fixes #14304

pitkajuh avatar Apr 01 '24 14:04 pitkajuh

Can one of the admins verify this patch?

phsft-bot avatar Apr 01 '24 14:04 phsft-bot

Changes ok.

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

ferdymercury avatar Apr 14 '24 12:04 ferdymercury

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 avatar Apr 14 '24 13:04 pitkajuh

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

Done.

pitkajuh avatar Apr 14 '24 14:04 pitkajuh

Test Results

     9 files       9 suites   1d 16h 55m 38s :stopwatch:  2 635 tests  2 635 :white_check_mark: 0 :zzz: 0 :x: 22 356 runs  22 356 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 61618a17.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Apr 14 '24 18:04 github-actions[bot]

Related post: https://root-forum.cern.ch/t/numerical-derivative-from-array

ferdymercury avatar Apr 18 '24 10:04 ferdymercury

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.

pitkajuh avatar May 10 '24 13:05 pitkajuh

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

ferdymercury avatar May 10 '24 17:05 ferdymercury