-
Notifications
You must be signed in to change notification settings - Fork 72
condense doc for sqrt into single docstring #2226
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
base: master
Are you sure you want to change the base?
Conversation
|
You now removed mentions of the docstrings from the manual (the md pages), but you additionally need to remove the docstrings themselves.
Please have a look at #2186. I would suggest to put your generic docstring right below the generic one added there for |
Got it, thanks! |
|
That's already looking quite good, thanks! One thing that is missing is adding both My suggestion for the moment would be to add a section In there, you would write something like this (untested): ## Square root
Some rings allow computing square roots.
```@docs
is_square
sqrt(::NCRingElem)
```
I am sure this can be improved, but we need to start somewhere. |
thanks for the suggestion, I tried to improve it a bit please let me know if the implementation matches the pattern you'd like it here. |
|
@varuntrehan7 this is marked as a draft -- do you have anything you still plan to do with this then? Otherwise, please just use the "Ready for review" button. |
docs/src/ring.md
Outdated
| while others only implement `is_square`. If `check=false`, the square root | ||
| check may be omitted. | ||
|
|
||
| The following entries link to the generic interface documentation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstrings follow below, and IMHO there is no point for this redundancy.
| while others only implement `is_square`. If `check=false`, the square root | |
| check may be omitted. | |
| The following entries link to the generic interface documentation: | |
| while others only implement `is_square`. |
docs/src/ring.md
Outdated
| Many rings in AbstractAlgebra provide functionality for detecting and computing | ||
| square roots. Two functions form the basic interface: | ||
|
|
||
| - `is_square(a)` – return `true` if the element `a` is a square in its ring. | ||
| - `sqrt(a; check=true)` – return a square root of `a`, when available. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I am not sure about "Many" -- how do you count this
- it's not just about rings in AbstractAlgebra
- the list of functions is redundant with the docstrings that follow right below.
| Many rings in AbstractAlgebra provide functionality for detecting and computing | |
| square roots. Two functions form the basic interface: | |
| - `is_square(a)` – return `true` if the element `a` is a square in its ring. | |
| - `sqrt(a; check=true)` – return a square root of `a`, when available. | |
| Rings may implement functionality for detecting and computing square roots. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay got it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2226 +/- ##
=======================================
Coverage 88.00% 88.00%
=======================================
Files 127 127
Lines 31803 31803
=======================================
Hits 27989 27989
Misses 3814 3814 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I was able to clear up the docs from various .md files ( if this was not what was asked please let me know I will undo it) and I am assuming the next step is that there needs to be a generic docstring wrapper for sqrt that goes somewhere which I am not sure where, please let me know. Thanks!