Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stdlib_selection: correction of typos and addition of some checks #585

Merged
merged 6 commits into from
Dec 8, 2021

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.

src/stdlib_selection.fypp Outdated Show resolved Hide resolved
@jvdp1 jvdp1 requested review from milancurcic and awvwgk 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants