#30230 closed defect (fixed)

Fix docstrings in sage/coding/linear_rank_metric.py

Reported by: fbissey Owned by:
Priority: major Milestone: sage-9.2
Component: documentation Keywords:
Cc: strogdon, slelievre Merged in:
Authors: Steven Trogdon Reviewers: Samuel Lelièvre
Report Upstream: N/A Work issues:
Branch: e43b5c2 (Commits, GitHub, GitLab) Commit: e43b5c28f3b1e067c7d4ea51bdea737efb4952df
Dependencies: #21226 Stopgaps:

Status badges

Description (last modified by slelievre)

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

Change History (37)

comment:1 Changed 16 months ago by strogdon

  • Branch set to u/strogdon/pdfdocs

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.

comment:2 Changed 16 months ago by fbissey

Can you push?

comment:3 Changed 16 months ago by git

  • Commit set to ddaba54f6d69276ed1b9c60193ace839542eb5a9

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

comment:4 Changed 16 months ago by strogdon

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

comment:5 Changed 16 months ago by git

  • Commit changed from ddaba54f6d69276ed1b9c60193ace839542eb5a9 to 451dbcab34a4c0a4dc84563be7aac4d545e90e00

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

451dbcaraw string fixes

comment:6 Changed 16 months ago by strogdon

I think we're there now.

comment:7 Changed 16 months ago by fbissey

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

comment:8 Changed 16 months ago by strogdon

  • Authors set to Steven Trogdon
  • Status changed from new to needs_review

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

comment:9 Changed 16 months ago by fbissey

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

comment:10 Changed 16 months ago by slelievre

  • Reviewers set to Samuel Lelièvre
  • Status changed from needs_review to needs_work

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.

comment:11 Changed 16 months ago by git

  • Commit changed from 451dbcab34a4c0a4dc84563be7aac4d545e90e00 to f8841602b9b9f3a4f4b17d693c32df5538a93a1e

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

f884160Add raw string markers and update Sphinx markup syntax

comment:12 Changed 16 months ago by strogdon

  • Status changed from needs_work to needs_review

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

comment:13 Changed 16 months ago by slelievre

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.

comment:14 Changed 16 months ago by fbissey

  • Description modified (diff)
  • Status changed from needs_review to positive_review
  • Summary changed from pdf doc build breakage in sage/coding/linear_rank_metric.py to QA in sage/coding/linear_rank_metric.py

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.

comment:15 Changed 16 months ago by slelievre

  • Description modified (diff)
  • Summary changed from QA in sage/coding/linear_rank_metric.py to Fix docstrings in sage/coding/linear_rank_metric.py

comment:16 Changed 16 months ago by slelievre

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.

comment:17 Changed 16 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:18 Changed 16 months ago by fbissey

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

comment:19 follow-up: Changed 16 months ago by strogdon

perhaps #30085

comment:20 in reply to: ↑ 19 ; follow-up: Changed 16 months ago by fbissey

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.

comment:21 in reply to: ↑ 20 Changed 16 months ago by strogdon

Replying to fbissey:

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.

comment:22 Changed 16 months ago by strogdon

Maybe fixed by #21226 and commit https://git.sagemath.org/sage.git/commit?id=899d3900180c956773f3d34c53b96483aa95c57f. But why make beta unicode? I don't think that ticket fixed the raw string markers.

comment:23 Changed 16 months ago by fbissey

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.

comment:24 Changed 16 months ago by strogdon

Wait 'til the next beta and then correct things?

comment:25 Changed 16 months ago by fbissey

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.

comment:26 Changed 16 months ago by git

  • Commit changed from f8841602b9b9f3a4f4b17d693c32df5538a93a1e to e43b5c28f3b1e067c7d4ea51bdea737efb4952df

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

comment:27 Changed 16 months ago by strogdon

  • Dependencies set to #21226
  • Status changed from needs_work to needs_review

Let me know if I've messed things up.

comment:28 Changed 16 months ago by fbissey

Somehow you have lost the unicode beta.

comment:29 follow-up: Changed 16 months ago by 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.

Last edited 16 months ago by strogdon (previous) (diff)

comment:30 Changed 16 months ago by dimpase

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

comment:31 in reply to: ↑ 29 ; follow-up: Changed 16 months ago by 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.

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.

comment:32 in reply to: ↑ 31 Changed 16 months ago by strogdon

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.

Last edited 16 months ago by strogdon (previous) (diff)

comment:33 Changed 16 months ago by fbissey

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.

comment:34 Changed 16 months ago by fbissey

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

comment:35 Changed 16 months ago by fbissey

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

comment:36 Changed 16 months ago by strogdon

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

comment:37 Changed 16 months ago by vbraun

  • Branch changed from u/strogdon/pdfdocs to e43b5c28f3b1e067c7d4ea51bdea737efb4952df
  • Resolution set to fixed
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.