Added Pure Python Implementation of Secant Root Finding

[iglesias-cardinale]

Added Pure Python Implementation of Secant Root Finding Method to calculus.py and added unit tests to test_calculus.py

Linting

• 10/10

Contents

• Updated calculus.py with secant_pure_pythonwhich takes in as arguments the function for which we wish to find zeros (func), initial guess at the location of the zeros (x0 and x1), the arguments of func (args) and the maximum number of iterations before the function gives up (maxiter). Returns a dictionary in the following format: {"root": None or root, "converged": True or False, "iterations": Number of iterations}
  • Updated test_calculus.py with three unit tests. The first tests if both secant method implementations are equivalent, the second tests if the scipy method has a valid result for non-convergence, and the third tests if the pure python method finds the correct root.

Reviewer

• @avgagliardo again seems like a good option. Maybe @zbpetersbuf or @ojha-aditya if they're in it for the love of the game

[ojha-aditya]

I can take this up if it is open

[zbpetersbuf]

go right ahead @ojha-aditya

[ojha-aditya]

go right ahead @ojha-aditya

Alright. I would self assign the review then?

[iglesias-cardinale]

The PR looks good to merge. Only one warning of RuntimeToleranceReached in pytest shows up. If that can be resolved before merging that will be good. Not exactly sure why it arises. If resolving that is not possible/necessary then I will approve without that too.

I think that happens on that test because the runtime for the test was maxed out. But it happens in the test to check for non-convergence so exceeding the runtime is inherently part of the check. There might be a way around it but I haven't found any obvious options. I can keep digging around if we want to get it resolved before merge.

[laserlab]

The PR looks good to merge. Only one warning of RuntimeToleranceReached in pytest shows up. If that can be resolved before merging that will be good. Not exactly sure why it arises. If resolving that is not possible/necessary then I will approve without that too.

I think that happens on that test because the runtime for the test was maxed out. But it happens in the test to check for non-convergence so exceeding the runtime is inherently part of the check. There might be a way around it but I haven't found any obvious options. I can keep digging around if we want to get it resolved before merge.

Maybe you can just use a really short timeout for the test.

[iglesias-cardinale]

The PR looks good to merge. Only one warning of RuntimeToleranceReached in pytest shows up. If that can be resolved before merging that will be good. Not exactly sure why it arises. If resolving that is not possible/necessary then I will approve without that too.

I think that happens on that test because the runtime for the test was maxed out. But it happens in the test to check for non-convergence so exceeding the runtime is inherently part of the check. There might be a way around it but I haven't found any obvious options. I can keep digging around if we want to get it resolved before merge.

Maybe you can just use a really short timeout for the test.

I experimented with this, but I believe that it happens regardless of the time/max iterations. I tried all the way down to maxiter=2 and still got the warning. I think it's just inherent in this type of test since what we're checking for is that the function times out before converging. In my most recent commit I just told pytest to ignore the warning for this test., but I'm happy to remove that if it's not acceptable.

[ojha-aditya]

Should I approve it in that case @laserlab as this would now be needed for the data presentation members? Minor functionality can be improved later as well maybe?

[laserlab]

I'm fine with purest to ignore that warning and maybe we can figure out later. Go ahead and merge

[zbpetersbuf]

@ojha-aditya if I merge before you accept the review you will not get points to my understanding