Update overlap.py to fix_factorial2 #309
Conversation
@D-TheProgrammer and I, think we solved the fix_factiorial issue#308
There was a problem hiding this comment.
This looks good to me. I've tagged @FarnazH as she did more work on this, so I'd wait a couple days for her to glance at it before merging.
|
Hi @PaulWAyers and @FarnazH , @LudoRNLT and I wanted to know if you have any updates regarding our fix and if it still suits you for the merge. |
|
I think @FarnazH is busy. Perhaps @tovrstra or @marco-2023 has time to do a quick review. |
|
@D-TheProgrammer: can you explain the error you were getting and that was fixed with this change? I just ran all unit tests without this change, and they pass (except for the problem fixed in #310). Edit: sorry found it #308 |
| for x in range(len(n)): | ||
| x = n[x] | ||
| out = scipy.special.factorial2(x, exact) #compute on each individual element x of the array n |
There was a problem hiding this comment.
Shouldn't this loop be dedented by four spaces?
There was a problem hiding this comment.
I'm sorry for my initial confusion. I'd like to request a few improvements:
- Major: Please add unit tests for the cases you're fixing. The issue reported in Fix Factorial2 #308 is not covered by our unit tests at the moment. Unit tests for the following cases (once with
exact=Trueand once withexact=False) would be useful:- integer arguments
0,1,2and3 - float arguments
0.0,1.0,2.0and3.0 - integer array argument
np.array([0, 1, 2, 3]) - float array argument
np.array([0.0, 1.0, 2.0, 3.0])
- integer arguments
- Minor: Please avoid comments at the end of a line. Put them on the line before instead. Also, don't add comments with little added value, such as translating a line of Python to a human-readable sentence. Comments are more useful when they explain the intent of the code, typically of a block consisting of multiple lines, which is different from a literal human translation of the code.
Thanks!
|
I'm closing this one because a more recent pull request was made: #319 |
@D-TheProgrammer and I, think that we solved the factorial error. Let us know if we have to do this but in a different way or use another syntax.