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

Fix docstrings in sage/coding/linear_rank_metric.py #30230

Closed
kiwifb opened this issue Jul 27, 2020 · 46 comments
Closed

Fix docstrings in sage/coding/linear_rank_metric.py #30230

kiwifb opened this issue Jul 27, 2020 · 46 comments

Comments

@kiwifb
Copy link
Member

kiwifb commented Jul 27, 2020

We fix a few docstring issues in the file sage/coding/linear_rank_metric.py:

  • use raw strings where needed: """ ... """ -> r""" ... """
  • fix latex typos: beta -> \beta; F_q{q^m} -> F_{q^m}
  • add missing :: to introduce example blocks or test blocks
  • fix verb forms in docstring first sentence: "Returns" -> "Return"

As reported in sage-on-gentoo github issue 593, the lack of raw strings caused errors when building the pdf docs when there were backslashes in LaTeX formulas:

! Package inputenc Error: Unicode character ^^H (U+0008)
(inputenc)                not set up for use with LaTeX.

See the inputenc package documentation for explanation.
Type  H <return>  for immediate help.
 ...                                              
                                                  
l.4655 \(1,^^H
              eta,\ldots,^^Heta^{sm}\) be the power basis that SageMath uses to

? 
! Emergency stop.
 ...                                              
                                                  
l.4655 \(1,^^H
              eta,\ldots,^^Heta^{sm}\) be the power basis that SageMath uses to

!  ==> Fatal error occurred, no output PDF file produced!
Transcript written on coding.log

Depends on #21226

CC: @strogdon @slel

Component: documentation

Author: Steven Trogdon

Branch/Commit: e43b5c2

Reviewer: Samuel Lelièvre

Issue created by migration from https://trac.sagemath.org/ticket/30230

@kiwifb kiwifb added this to the sage-9.2 milestone Jul 27, 2020
@strogdon
Copy link
Contributor

comment:1

A few typo corrections in addition to adding raw string markers. It's not clear why the failure now appears since the code in sage/coding/linear_rank_metric.py hasn't been recently changed. Perhaps the sphinx upgrade, #30001 exposed the issue. It would be good if someone could review the code in sage/coding/linear_rank_metric.py to determine if additional changes are warranted.

@strogdon
Copy link
Contributor

Branch: u/strogdon/pdfdocs

@kiwifb
Copy link
Member Author

kiwifb commented Jul 27, 2020

comment:2

Can you push?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 27, 2020

Commit: ddaba54

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 27, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

@strogdon
Copy link
Contributor

comment:4

had to change my public key but that which was pushed is not correct.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 27, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

451dbcaraw string fixes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 27, 2020

Changed commit from ddaba54 to 451dbca

@strogdon
Copy link
Contributor

comment:6

I think we're there now.

@kiwifb
Copy link
Member Author

kiwifb commented Jul 27, 2020

comment:7

That's one more typo than I had spotted. We should wait for the approval of Samuel Lelievre before going to positive review.

@strogdon
Copy link
Contributor

Author: Steven Trogdon

@strogdon
Copy link
Contributor

comment:8

I did notice that there was one more string that needed to be a raw string, but I don't believe it impacted anything.

@kiwifb
Copy link
Member Author

kiwifb commented Jul 27, 2020

comment:9

I think sage doc in general needs more raw strings but this is not my decision.

@slel
Copy link
Member

slel commented Jul 27, 2020

comment:10

Thanks for fixing this. I consider all docstrings should be raw!

While we're at it would you

  • add the missing :: at lines 396, 418, 426, 433, 464, 521
  • remove the empty line at line 316

Or you can leave that for a different ticket and set to positive review on my behalf.

There would also be a few docstring first lines to change:

  • "Returns" -> "Return"
  • "Constructs" -> "Construct"
  • "Initializes" -> "Initialize"
  • "Tests" -> "Test"
    but that's cosmetic and might even more belong to a different ticket.

@slel
Copy link
Member

slel commented Jul 27, 2020

Reviewer: Samuel Lelièvre

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 27, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

f884160Add raw string markers and update Sphinx markup syntax

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 27, 2020

Changed commit from 451dbca to f884160

@strogdon
Copy link
Contributor

comment:12

Let's see if I got everything. Feel free to make any changes.

@slel
Copy link
Member

slel commented Jul 27, 2020

comment:13

Since some doctests will be run for the first time (with the new ::),
let's hear from the bots.

If the bots are happy, positive review.

@kiwifb
Copy link
Member Author

kiwifb commented Jul 28, 2020

comment:14

I changed the ticket title and description to reflect the scope creep. I note that some bots do fine but one has failed because of a lack of disk space.

The bots which have run are happy enough so I move this to positive review which should lead to further bot testing.

@kiwifb

This comment has been minimized.

@slel
Copy link
Member

slel commented Jul 28, 2020

comment:16

I amended the ticket summary and description a bit more, hope that's ok:
I learned a lot about Sage by watching Trac and lists, which was not always easy,
so I try to make ticket summaries and descriptions as helpful as I can to learners.

The happy patchbots still have pyflakes warnings about unused imports,
but let's get this in and those imports can be cleaned up in a different ticket.

@vbraun
Copy link
Member

vbraun commented Jul 28, 2020

comment:17

Merge conflict

@kiwifb
Copy link
Member Author

kiwifb commented Jul 28, 2020

comment:18

Does any one know which other ticket is also touching sage/coding/linear_rank_metric.py?

@strogdon
Copy link
Contributor

comment:19

perhaps #30085

@kiwifb
Copy link
Member Author

kiwifb commented Jul 28, 2020

comment:20

Replying to @strogdon:

perhaps #30085

Well, it created the file as far as I see it. And was included in the latest beta. In other word, We are truly fixing something introduced in the latest beta.

But may there is another ticket addressing some of the points we are looking in this ticket.

@strogdon
Copy link
Contributor

comment:21

Replying to @kiwifb:

Replying to @strogdon:

perhaps #30085

Well, it created the file as far as I see it. And was included in the latest beta. In other word, We are truly fixing something introduced in the latest beta.

But may there is another ticket addressing some of the points we are looking in this ticket.

You're right. It was closed 3 days ago. I only looked at that. This then explains the pdf docs failing.

@strogdon
Copy link
Contributor

comment:22

Maybe fixed by #21226 and commit sagemath/sagetrac-mirror@899d390. But why make beta unicode? I don't think that ticket fixed the raw string markers.

@kiwifb
Copy link
Member Author

kiwifb commented Jul 29, 2020

comment:23

That looks like our culprit. Just been merged. The use of unicode in these case usually boils down to "because I can". I don't know how portable it is. And yes it doesn't fix the raw string markers, or the missing ":" or the various spelling/grammar mistakes. Just the use of \beta and some doctests change.

@strogdon
Copy link
Contributor

comment:24

Wait 'til the next beta and then correct things?

@kiwifb
Copy link
Member Author

kiwifb commented Jul 29, 2020

comment:25

That or merge the ticket in this one and mark it as depending on that ticket (just in case it gets unmerged). I don't have a preference either way. Sometimes a merge is complicated but this one is easily manageable I'd say.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2020

Changed commit from f884160 to e43b5c2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

150d834Example for coding.linear_code_no_metric.__init__
5f32a03Merge branch 'u/dimpase/coding/linear_rank_metric' of git://trac.sagemath.org/sage into 21226_abstract_rank_metric
4a770f8Fix `__eq__` and `__ne__` for AbstractLinearCodeNoMetric, and remove it from AbstractLinearCode
9ef1e9cAdd doctest to coding.linear_rank_metric.LinearRankMetricCodeNearestNeighborDecoder.decode_to_code
5f46031Make doctests of sage.coding.linear_rank_metric run 50 times faster
899d390make \beta unicode
e43b5c2merge ticket 21226 into ticket 30230

@strogdon
Copy link
Contributor

comment:27

Let me know if I've messed things up.

@strogdon
Copy link
Contributor

Dependencies: #21226

@kiwifb
Copy link
Member Author

kiwifb commented Jul 29, 2020

comment:28

Somehow you have lost the unicode beta.

@strogdon
Copy link
Contributor

comment:29

The original branch here had \beta inside raw strings which allowed the pdf docs to build. I left things as they were, i.e. I replaced unicode beta with \beta since unicode is not needed inside raw strings. At least this is what I was trying to do. I don't know how to check to see if this branch can successfully be merged on top of #21226.

@dimpase
Copy link
Member

dimpase commented Aug 1, 2020

comment:30

I don't understand what's going on - it seems as if the last commit of #21226 did not make it into the beta6.

@dimpase
Copy link
Member

dimpase commented Aug 2, 2020

comment:31

Replying to @strogdon:

The original branch here had \beta inside raw strings which allowed the pdf docs to build. I left things as they were, i.e. I replaced unicode beta with \beta since unicode is not needed inside raw strings. At least this is what I was trying to do. I don't know how to check to see if this branch can successfully be merged on top of #21226.

what's wrong with unicode beta? They're displayed quite well in 21st century setups.

By the way, did you check that pdf docs build with your changes? I spent some time trying to fix that pdf failing problem without resorting to unicode, gave up and added unicode, which made them work. The reason it ever popped up is that something went wrong in the release process and the commit which fixed that issue didn't make it in.

@strogdon
Copy link
Contributor

strogdon commented Aug 2, 2020

comment:32

Replying to @dimpase:

Replying to @strogdon:

The original branch here had \beta inside raw strings which allowed the pdf docs to build. I left things as they were, i.e. I replaced unicode beta with \beta since unicode is not needed inside raw strings. At least this is what I was trying to do. I don't know how to check to see if this branch can successfully be merged on top of #21226.

what's wrong with unicode beta? They're displayed quite well in 21st century setups.

I suppose nothing is wrong with using a unicode beta. I'll let others decide as to whether unicode should be used when instead a raw string marker can be added with the usual \beta.

By the way, did you check that pdf docs build with your changes? I spent some time trying to fix that pdf failing problem without resorting to unicode, gave up and added unicode, which made them work. The reason it ever popped up is that something went wrong in the release process and the commit which fixed that issue didn't make it in.

Yes, this was tested (see ​sage-on-gentoo github issue 593). And it was tested on vanilla Sage as well. Yes is was 9.2.beta6 where I discovered it. Personally, I think using raw string markers is easier to implement.

@kiwifb
Copy link
Member Author

kiwifb commented Aug 2, 2020

comment:33

Using unicode beta certainly helps getting rid of problems with \b. But ultimately using raw string is the proper thing to do. And one doesn't prevent the other I guess.

@kiwifb
Copy link
Member Author

kiwifb commented Aug 2, 2020

comment:34

9.2.beta7 is available now on github, let's rebase on that.

@kiwifb
Copy link
Member Author

kiwifb commented Aug 3, 2020

comment:35

Volker is merging, let's not do anything until he reports.

@strogdon
Copy link
Contributor

strogdon commented Aug 3, 2020

comment:36

It merged cleanly and the associated commit looks to be correct.

@vbraun
Copy link
Member

vbraun commented Aug 7, 2020

Changed branch from u/strogdon/pdfdocs to e43b5c2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants