Skip to content

Add documentation for reverse() #148

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

Merged
merged 3 commits into from
Feb 24, 2020
Merged

Add documentation for reverse() #148

merged 3 commits into from
Feb 24, 2020

Conversation

mcol
Copy link
Contributor

@mcol mcol commented Feb 9, 2020

Submission Checklist

  • Builds locally
  • Declare copyright holder and open-source license: see below

Summary

This adds docs for reverse, which was added in stan-dev/math#1650. I'm not entirely convinced of the section titles I used, I followed what was around and couldn't think of something better.

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):

Marco Colombo

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

Copy link
Member

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

One question about scope and some changes around phrasing.

@@ -409,3 +410,21 @@ Number of components of v less than v[s]
`int` **`rank`**`(int[] v, int s)`<br>\newline
Number of components of v less than v[s]

## Reversing Functions {#reversing-functions}

Reversing allows to create a copy of array in which elements are in reverse
Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite grammatical and it's not a copy. Also, we want to make the doc more active. So this should be:

Stan provides functions to create a new array by reversing the order of elements in an existing array.  For example, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the much improved wording!

\index{{\tt \bfseries reverse }!{\tt (int[] v): int[]}|hyperpage}

`int[]` **`reverse`**`(int[] v)`<br>\newline
Create a copy of the array with the elements in reverse order.
Copy link
Member

Choose a reason for hiding this comment

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

Does this only work for integer and real arrays? It should work for any array if possible, or if not, at least all arrays up to a given number of dimensions. If this PR doesn't create a generic reverse function, please create a new issue with that goal. Then the doc can just use T[] reverse (T[] x).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation does not restrict what can be reversed, so I'll change that to use T[].

\index{{\tt \bfseries reverse }!{\tt (vector v): vector}|hyperpage}

`vector` **`reverse`**`(vector v)`<br>\newline
Create a copy of the vector with the elements in reverse order.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a copy any more than sorting is a copy. It returns a new vector containing the elements of the argument vector in reverse order.

\index{{\tt \bfseries reverse }!{\tt (real[] v): real[]}|hyperpage}

`real[]` **`reverse`**`(real[] v)`<br>\newline
Create a copy of the array with the elements in reverse order.
Copy link
Member

Choose a reason for hiding this comment

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

A copy would have the elements in the same order. This function returns a new array with the argument array's elements in reverse order.

mitzimorris
mitzimorris previously approved these changes Feb 10, 2020
Copy link
Member

@mitzimorris mitzimorris 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, many thanks!

@mitzimorris mitzimorris dismissed their stale review February 10, 2020 22:45

defer to Bob

@mcol mcol requested a review from bob-carpenter February 24, 2020 13:26
Copy link
Member

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

Other than title case, it looks great. I'm approving and leaving it up to you what to do with title case. At some point, it'd be nice to fix all of our doc for consistency on this.

@@ -334,7 +335,7 @@ Any mismatches will cause an error to be thrown.
x3 = append_array(x1, x2);
```

## Sorting functions {#sorting-functions}
## Sorting Functions {#sorting-functions}
Copy link
Member

Choose a reason for hiding this comment

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

This should be Sorting functions not Sorting Functions.

  • # XXX uses title punctuation for XXX (all caps other than "function" words [prepositions, determiners, auxliary verbs, other particles, etc.])
  • ## XXX should use sentence punctuation for XXX (initial caps, not all caps)

@@ -409,3 +410,16 @@ Number of components of v less than v[s]
`int` **`rank`**`(int[] v, int s)`<br>\newline
Number of components of v less than v[s]

## Reversing Functions {#reversing-functions}
Copy link
Member

Choose a reason for hiding this comment

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

fix case

Copy link
Member

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

Thanks.

@bob-carpenter bob-carpenter merged commit bd1d16b into master Feb 24, 2020
@mcol mcol deleted the add-reverse branch February 24, 2020 21:14
@rok-cesnovar rok-cesnovar mentioned this pull request Apr 29, 2020
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.

3 participants