Fix # optional tags

[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​

[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.

[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.

[kcrisman]

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?

[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.

[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.

[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.

[kcrisman]

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...

[kcrisman]

But otherwise positive review.

[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.

[kcrisman]

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.

[jpflori]

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

[jdemeyer]

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

# optional - time

tags.

[jdemeyer]

New version of 14184_manual.patch​ needs review.

[kcrisman]

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