Opened 7 years ago

Closed 7 years ago

#14184 closed defect (fixed)

Fix # optional tags

Reported by: jdemeyer Owned by: mvngu
Priority: major Milestone: sage-5.8
Component: doctest coverage Keywords:
Cc: jpflori, dimpase Merged in: sage-5.8.beta3
Authors: Jeroen Demeyer Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Execute

while read file; do

if [ -h $file ]; then
    continue
fi

sed -i -f /dev/stdin $file <<'EOF'

/sage: .*#/ {
s/\(#.*\) needs optional/\1 optional -/i;
s/\(#.*\) requires optional/\1 optional -/i;
s/\(#\) requires[-:, ]*\([A-Za-z0-9_]*\)[-:, ]*optional/\1 optional - \2/i;
s/\(#.*\) and optional/\1; optional/i;
s/\(#.*\) \(random\) optional/\1 \2; optional/i;
s/\(#.*\) \(long time\) optional/\1 \2; optional/i;
s/\(#.*[^; ]\)[; ]  *optional/\1; optional/i;

s/\([#;,]\) *optional and /\1 optional; /i;

s/\([#;,]\) *optional[-:, ]* needs /\1 optional - /i;
s/\([#;,]\) *optional[-:, ]* requires /\1 optional - /i;
s/\([#;,]\) *optional[-: ]* the /\1 optional - /i;
s/\([#;,]\) *optional[-: ]* GAP packages/\1 optional - gap_packages/i;
s/\([#;,]\) *optional[-: ]* GAP package \([A-Za-z0-9']*\)/\1 optional - gap_packages (\2 package)/i;
s/\([#;,]\) *optional[-: ]* GAP \(optional \)*database[s]*/\1 optional - database_gap/i;
s/\([#;,]\) *optional[-: ]* package/\1 optional -/i;
s/\([#;,]\) *optional[-: ]* \([A-Za-z0-9_]*\) command/\1 optional - \2/i;
s/\([#;,]\) *optional[-: ]* \([A-Za-z0-9_]*\) package/\1 optional - \2/i;
s/\([#;,]\) *optional[-: ]* \([A-Za-z0-9_]*\) installed/\1 optional - \2/i;
s/\([#;,]\) *optional[-: ]* \([A-Za-z0-9_]*\) and /\1 optional - \2 /i;
s/\([#;,]\) *optional[-: ]* \([A-Za-z0-9_]*\) \(not tested\)/\1 optional - \2; \3/i;

s/\([#;,]\) *optional[-: ]* internet connection/\1 optional - internet/i;
s/\([#;,]\) *optional[-: ]* database_hohel/\1 optional - database_kohel/i;
s/\([#;,]\) *optional[-: ]* Octave/\1 optional - octave/;
s/\([#;,]\) *optional[-: ]* Axiom/\1 optional - axiom/;

s/\([#;,]\) *optional[-: ]* \(.*\)'\(.*\)'/\1 optional - \2\3/i;

T;

s/\([#;,] *optional - .*[^# ]\)[# ] *$/\1/i;
s/\([#;,] *optional\)[-: ][-: ]*$/\1/i;
}

EOF
done < <(find devel/sage/sage devel/sage/doc/[a-z][a-z] -name '*.py*' -o -name '*.rst')

# Fix specific files
sed -i 's/# optional/# optional - gap_packages (Guava package)/' devel/sage/sage/coding/guava.py

Apply 14184_optional.patch and 14184_manual.patch and 14184_manual2.patch

Attachments (3)

14184_optional.patch (103.1 KB) - added by jdemeyer 7 years ago.
14184_manual.patch (4.5 KB) - added by jdemeyer 7 years ago.
14184_manual2.patch (10.2 KB) - added by jdemeyer 7 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:7 follow-up: Changed 7 years ago by kcrisman

Isn't this exactly the kind of huge intrusive patch we've been trying to avoid? ;-) That said, it would be really nice to have standardization for people to look at, so I'm not opposed to this.

Stuff to fix and/or think about doing as long as this ticket is opened.

  • The sage/coding/guava.py should have the same message as the ones in sage/coding/code_bounds.py and sage/coding/linear_code.py
  • # long time; optional - internet and in another place, reversed - I assume that won't matter, but just asking in case
  • # optional - database_hohel - I don't know of a Hohel in Sage development, but I bet the database_kohel spkg is what is meant :-)
  • Did we change the convert ones to ImageMagick, or vice versa? Anyway, that should be standardized if it wasn't already elsewhere (this feels familiar)
  • I'm not sure why something can depend on dot2tex but not graphviz, and even more weirdly, graphviz is experimental, but not dot2tex ?!?!? Though I guess it is okay...
  • sometimes octave, sometimes Octave - does capitalization matter? Same with Axiom.
  • somewhat random - is that an official designation?
  • < 1/100th of a second - well, I guess it might be! I guess we can leave that.
  • What is the time command? The one that we always have? I'm confused here.
    sage: q = qsieve(n, block=False, time=True)           # optional - time 
    
  • The sage/lfunctions/sympow.py test seems ... odd. Should it instead be long test? Note that the previous command just says optional, with no indication of why.
  • # optional - FES, random - does it matter if random is before or after? I would think this is going to look for the random spkg. Same with # optional; randomly growing.

Otherwise looks good - I did look at all of them by hand, not the script. Though obviously I am not going to try to test all of these optional tests by hand.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by jdemeyer

Replying to kcrisman:

Isn't this exactly the kind of huge intrusive patch we've been trying to avoid? ;-)

Not really. Huge intrusive patches are okay as long as they serve a purpose. Removing whitespace all over the place doesn't serve a purpose, this patch certainly does.

This patch is mainly motivated by #12415, which has different (more strict) parsing for # optional, leading to less false positives (but also less true positives, hence this patch).

  • The sage/coding/guava.py should have the same message as the ones in sage/coding/code_bounds.py and sage/coding/linear_code.py

OK, I'll have a look.

  • # long time; optional - internet and in another place, reversed - I assume that won't matter, but just asking in case

Doesn't matter.

  • # optional - database_hohel - I don't know of a Hohel in Sage development, but I bet the database_kohel spkg is what is meant :-)

OK, so that's a typo.

  • Did we change the convert ones to ImageMagick, or vice versa? Anyway, that should be standardized if it wasn't already elsewhere (this feels familiar)

I have no idea.

  • I'm not sure why something can depend on dot2tex but not graphviz, and even more weirdly, graphviz is experimental, but not dot2tex ?!?!? Though I guess it is okay...

I have no idea what these packages are.

  • sometimes octave, sometimes Octave - does capitalization matter? Same with Axiom.

OK, changing to all lowercase.

  • somewhat random - is that an official designation?

The doctester looks for "random", so it will be recognized. I think it's fine.

  • < 1/100th of a second - well, I guess it might be! I guess we can leave that.

This can be seen as "just a comment" without further meaning.

  • What is the time command? The one that we always have? I'm confused here.

I have no idea.

  • The sage/lfunctions/sympow.py test seems ... odd. Should it instead be long test? Note that the previous command just says optional, with no indication of why.

True, it's odd. I think it means that you need to run sympow first to create some tables, but I don't know more.

  • # optional - FES, random - does it matter if random is before or after? I would think this is going to look for the random spkg. Same with # optional; randomly growing.

At least with the new doctester (which is what we're aiming for), optional - FES, random will only require FES.

Though obviously I am not going to try to test all of these optional tests by hand.

That is also most certainly not the point of this ticket.

comment:9 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 7 years ago by jdemeyer

  • Status changed from new to needs_review

comment:11 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

This patch is mainly motivated by #12415, which has different (more strict) parsing for # optional, leading to less false positives (but also less true positives, hence this patch).

I figured :)

  • Did we change the convert ones to ImageMagick, or vice versa? Anyway, that should be standardized if it wasn't already elsewhere (this feels familiar)

I have no idea.

I was right - we did change it to the program needed. search_src('ImageMagick') gets me a lot of hits, particularly in plot/animate.py

plot/animate.py:430:            sage: a.show()       # optional -- ImageMagick
plot/animate.py:435:            sage: a.show(iterations=3)    # optional -- ImageMagick
plot/animate.py:439:            sage: a.show(delay=50)        # optional -- ImageMagick

I think this needs to be uniformized. See #11170, where this was done, except apparently to the guys here (see also #10655 and #7981 for background). By the way, is the -- versus - important?

  • I'm not sure why something can depend on dot2tex but not graphviz, and even more weirdly, graphviz is experimental, but not dot2tex ?!?!? Though I guess it is okay...

I have no idea what these packages are.

It's ok, I think this is okay after looking at the various package lists.

  • somewhat random - is that an official designation?

The doctester looks for "random", so it will be recognized. I think it's fine.

Ok

  • < 1/100th of a second - well, I guess it might be! I guess we can leave that.

This can be seen as "just a comment" without further meaning.

Just wanted to make sure that additional comments were okay in doctest lines.

  • What is the time command? The one that we always have? I'm confused here.

I have no idea.

Got it - see the line above them (which your script didn't catch)?

sage: v, t = qsieve(n, time=True)   # uses the sieve    (optional: time doesn't work on cygwin) 

This line should be changed to "look nice", and now we know why this was marked optional. It hardly seems fair to make something optional that only doesn't work on a not-yet-supported platform... maybe we should remove these as optional? I'll leave that to you.

  • The sage/lfunctions/sympow.py test seems ... odd. Should it instead be long test? Note that the previous command just says optional, with no indication of why.

True, it's odd. I think it means that you need to run sympow first to create some tables, but I don't know more.

I believe you are right. But the doctest makes no sense as an automatic doctest in that case. See the relevant note. In fact, I get the bizarre (newlines not rendered)

sage: a = sympow.L(EllipticCurve('11a'), 2, 16); a 
"Inertia Group is  C1 MULTIPLICATIVE REDUCTION\nConductor is 11\n**ERROR** P02L not found in param_data file\nIt can be added with './sympow -new_data 2'"

So I believe that both of them should probably be made optional - precomputations, or perhaps even better to add

sage: sympow('-new_data 2') # not tested

to the doctest and make them all not tested, since by definition someone would have to do something "by hand" to make them testable. Or do you have another creative solution? I can think of a couple, but they are annoying and probably pointless.

There should also be better error catching there, but I figure anyone using sympow knows what they are doing enough to read the documentation - or is that worth a ticket as well?

comment:12 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:13 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by jdemeyer

Replying to kcrisman:

I was right - we did change it to the program needed. search_src('ImageMagick') gets me a lot of hits, particularly in plot/animate.py

plot/animate.py:430:            sage: a.show()       # optional -- ImageMagick
plot/animate.py:435:            sage: a.show(iterations=3)    # optional -- ImageMagick
plot/animate.py:439:            sage: a.show(delay=50)        # optional -- ImageMagick

I think this needs to be uniformized.

Done, changed all to optional -- ImageMagick.

By the way, is the -- versus - important?

No, I used a double dash for consistency here.

sage: v, t = qsieve(n, time=True)   # uses the sieve    (optional: time doesn't work on cygwin) 

This line should be changed to "look nice", and now we know why this was marked optional.

Fixed.

I believe you are right. But the doctest makes no sense as an automatic doctest in that case. See the relevant note. In fact, I get the bizarre (newlines not rendered)

sage: a = sympow.L(EllipticCurve('11a'), 2, 16); a 
"Inertia Group is  C1 MULTIPLICATIVE REDUCTION\nConductor is 11\n**ERROR** P02L not found in param_data file\nIt can be added with './sympow -new_data 2'"

So I believe that both of them should probably be made optional - precomputations, or perhaps even better to add

sage: sympow('-new_data 2') # not tested

to the doctest and make them all not tested, since by definition someone would have to do something "by hand" to make them testable.

Done.

There should also be better error catching there, but I figure anyone using sympow knows what they are doing enough to read the documentation - or is that worth a ticket as well?

Technically, it's worth a ticket but I have no plans to work on it.

Changed 7 years ago by jdemeyer

comment:14 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:15 in reply to: ↑ 13 Changed 7 years ago by kcrisman

So I believe that both of them should probably be made optional - precomputations, or perhaps even better to add

sage: sympow('-new_data 2') # not tested

to the doctest and make them all not tested, since by definition someone would have to do something "by hand" to make them testable.

Done.

Note that technically ``sympow -new_data 2`` isn't needed; sage: sympow('-new_data 2') is just as good, and has the advantage of being able to be included in the doctests (# not tested, of course). May I ask for that very slight change in the "manual" patch - just so people don't think they have to go to the shell?

There should also be better error catching there, but I figure anyone using sympow knows what they are doing enough to read the documentation - or is that worth a ticket as well?

Technically, it's worth a ticket but I have no plans to work on it.

Nor do I.

comment:16 follow-up: Changed 7 years ago by jdemeyer

Judging from sage-devel, it seems that time does work on Cygwin now (probably it didn't in some earlier version), so I removed optional - time. I also clarified the SYMPOW precomputations as you suggested.

comment:17 in reply to: ↑ 16 Changed 7 years ago by kcrisman

  • Cc jpflori dimpase added

Replying to jdemeyer:

Judging from sage-devel, it seems that time does work on Cygwin now (probably it didn't in some earlier version), so I removed optional - time. I also clarified the SYMPOW precomputations as you suggested.

See this comment on Trac 9164. What do you think we should do? I'm almost wondering whether it's worth keeping your current patch and then opening a ticket acknowledging that this doesn't work on Cygwin...

comment:18 Changed 7 years ago by kcrisman

But otherwise positive review.

comment:19 follow-up: Changed 7 years ago by jpflori

I'd say leave this ticket as is and modify the code to try using time on Cygwin and suggesting installing the time package if it fails (or not using time=True) in another ticket.

comment:20 in reply to: ↑ 19 Changed 7 years ago by kcrisman

  • Status changed from needs_review to positive_review

Replying to jpflori:

I'd say leave this ticket as is and modify the code to try using time on Cygwin and suggesting installing the time package if it fails (or not using time=True) in another ticket.

So in other words, not re-enabling optional, with the realization that we'll have a purposely failing doctest on Cygwin. I don't really have a problem with this.

comment:21 Changed 7 years ago by jpflori

Exactly, but opening a follow up ticket. It's now #14202

comment:22 Changed 7 years ago by jdemeyer

Since the "time" problem has nothing to do with Cygwin, I will keep the

# optional - time

tags.

comment:23 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Changed 7 years ago by jdemeyer

comment:24 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

New version of 14184_manual.patch needs review.

comment:25 Changed 7 years ago by kcrisman

  • Status changed from needs_review to positive_review

Patchbot is confused... apply 14184_optional.patch and 14184_manual.patch

comment:26 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from positive_review to needs_work

comment:27 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

I found some more which I missed initially.

Changed 7 years ago by jdemeyer

comment:28 Changed 7 years ago by kcrisman

  • Status changed from needs_review to positive_review

I'm sure there will always be more...

comment:29 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.8.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.