Add function gcd #539
Add function gcd #539
Conversation
|
I appreciate tips about tests. More, less, special cases … |
awvwgk
added
reviewers needed
|
Thanks @Sideboard, the implementation looks good to me. Tests seem fine as well. We should add the FORD docstrings to the function, and the spec as well. You can look at other functions for examples. Let us know if you need help with that. General question for everybody, do we have a guide on when to put a function implementation in its own submodule and when to leave it in the module? I see that we have |
|
Ok, I can add them. I looked at clip for reference. Thought it was in the main file because of its size. The subfile question extends to the corresponding tests. |
|
The spec only says "integer" and "another integer" for now. |
A test with nested functions, maybe. |
Does this really test anything new? If Is there interest in a version of gcd that works with a rank-1 array for more than two arguments? |
What do you mean? Could you give an example, please? Edit: Something like |
Yes, that's what I mean. |
On the long term I think the public modules should contain only the interfaces, and all of the implementation should be placed in sub-modules. This would facilitate compiler vendors providing their own optimized implementations in the (distant?) future. It would also make it easier to swap between debug and release binaries by re-linking. I think the current reasoning was simply when launching a new module with a small function, it was just easier to prepare a single module. A second reason in favor of separating interface and implementation might be to use a stdlib library compiled by another Fortran compiler. In this case you would only need to compile the interface (see #530 (comment) and the discussion that follows). As an example, Intel MKL ships libraries that can be used with both Intel and gfortran compilers. @milancurcic, should I make a new issue to discuss this? We can then edit the developer docs in a new PR. |
|
Yes @ivan-pi, thanks, that would be good. |
|
Should anything be changed for this PR? |
There was a problem hiding this comment.
Looks great to me! I have left few comments, some of them are just open ended question for everyone. Thank you @Sideboard.
There was a problem hiding this comment.
Thanks @14NGiestas for the interesting link. There was a nice response to that blog post from Prof Daniel Lemire (@lemire).
Pending the small issues in the documentation, I approve merging this.
src/stdlib_math.fypp
Outdated
| !> | ||
| !> Version: experimental |
There was a problem hiding this comment.
I think a comment stating the (current) implementation uses the "Euclidean algorithm" would be helpful.
In the future a competing implementation might use another algorithm.
src/tests/math/test_stdlib_math.f90
Outdated
| call check(gcd(-9_int8, 6_int8) == 3_int8, 'gcd failed for gcd(-9, 6)', warn=.true.) | ||
| call check(gcd(9_int8, -6_int8) == 3_int8, 'gcd failed for gcd(9, -6)', warn=.true.) |
There was a problem hiding this comment.
Perhaps another test with negative a and b = 0 would be good, just to verify the limit case gcd(a,0) == abs(a) is indeed true.
There was a problem hiding this comment.
I added (0, -2). Now I see that it's not exactly what was asked for but probably close enough.
|
Is there a consensus on where the annotation should be, the interface or the implementation? |
|
Both descriptive (e.g. "Converts …") and imperative ("Convert …") formulations can be found among the function annotations. |
|
@milancurcic this still needs one more approval to be merged. @Sideboard small change is required, see this. |
|
I thought the approvals were invalidated when I pushed to the branch but a grey check mark means that the reviewer does not have write-access necessary for the merge. Should I request a review from someone with those rights? Who? |
|
Thanks all, let's merge it. |
Closes #515
a. Handle negative input as positive