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​

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