Skip to content

Conversation

jvdp1
Copy link
Member

@jvdp1 jvdp1 commented Dec 1, 2021

  • I corrected some typos in stdlib_selection.md
  • I added some checks to ensure that k in select(x, k, kth, letft, right) is within the interval [left: right].

ping @gareth-nx

@jvdp1 jvdp1 requested a review from gareth-nx December 1, 2021 21:13
Copy link
Contributor

@gareth-nx gareth-nx left a comment

Choose a reason for hiding this comment

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

Thanks for that, looks good.

@jvdp1 jvdp1 requested review from awvwgk and milancurcic December 1, 2021 22:06
@jvdp1 jvdp1 marked this pull request as ready for review December 1, 2021 22:06
Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Contributor

@gareth-nx gareth-nx left a comment

Choose a reason for hiding this comment

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

Thanks!

@ivan-pi
Copy link
Member

ivan-pi commented Dec 2, 2021

The error message now doesn't mention the condition that k be in the interval [l, r]. Maybe breaking into two if blocks would make it easier on the eye. But I don't have strong feelings about this.

@gareth-nx
Copy link
Contributor

I just noticed another minor issue in stdlib_selection.md. At line 26, there should be a new line before * select.

At the moment you can see we are missing a newline in the html view, just under the "Overview of the module" section. Interestingly this formatting issue doesn't occur with the github markdown view.

If you can add a newline as part of this pull request, great. Otherwise I can make a pull request later.

@jvdp1
Copy link
Member Author

jvdp1 commented Dec 3, 2021

The error message now doesn't mention the condition that k be in the interval [l, r]. Maybe breaking into two if blocks would make it easier on the eye. But I don't have strong feelings about this.

Thank you @ivan-pi . I modified the lines as follows:

error stop "select must have 1 <= left <= k <= right <= size(a)"; 
@jvdp1
Copy link
Member Author

jvdp1 commented Dec 3, 2021

I just noticed another minor issue in stdlib_selection.md. At line 26, there should be a new line before * select.

At the moment you can see we are missing a newline in the html view, just under the "Overview of the module" section. Interestingly this formatting issue doesn't occur with the github markdown view.

If you can add a newline as part of this pull request, great. Otherwise I can make a pull request later.

Done. Thank you @gareth-nx

@jvdp1 jvdp1 requested a review from ivan-pi December 4, 2021 08:55
@gareth-nx
Copy link
Contributor

@jvdp1 In case you make further changes here, could you please also add a suitable ford comment at the top of the stdlib_selection.fypp, such as:

!! Quickly find the k-th smallest value of an array, or the index of the k-th smallest value. 

This statement will then appear on the stdlib module html page: https://stdlib.fortran-lang.org/lists/modules.html

Thanks.

Copy link
Contributor

@gareth-nx gareth-nx left a comment

Choose a reason for hiding this comment

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

Thanks

@jvdp1
Copy link
Member Author

jvdp1 commented Dec 8, 2021

Mainly fixing typos and links for FORD. Thank you to the reviewers.

@jvdp1 jvdp1 merged commit 26b6f58 into fortran-lang:master Dec 8, 2021
@jvdp1 jvdp1 deleted the pr_selection branch December 22, 2021 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants