Opened 9 years ago
Last modified 9 years ago
#14184 closed defect
Fix # optional tags — at Version 26
Reported by: | jdemeyer | Owned by: | mvngu |
---|---|---|---|
Priority: | major | Milestone: | sage-5.8 |
Component: | doctest coverage | Keywords: | |
Cc: | jpflori, dimpase | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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
Change History (28)
comment:1 Changed 9 years ago by
- Description modified (diff)
comment:2 Changed 9 years ago by
- Description modified (diff)
comment:3 Changed 9 years ago by
- Description modified (diff)
comment:4 Changed 9 years ago by
- Description modified (diff)
comment:5 Changed 9 years ago by
- Description modified (diff)
comment:6 Changed 9 years ago by
- Description modified (diff)
comment:7 follow-up: ↓ 8 Changed 9 years ago by
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 11 Changed 9 years ago by
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 toImageMagick
, 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
, sometimesOctave
- does capitalization matter? Same withAxiom
.
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 therandom
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 9 years ago by
- Description modified (diff)
comment:10 Changed 9 years ago by
- Status changed from new to needs_review
comment:11 in reply to: ↑ 8 ; follow-up: ↓ 13 Changed 9 years ago by
- 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 toImageMagick
, 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 9 years ago by
- Description modified (diff)
comment:13 in reply to: ↑ 11 ; follow-up: ↓ 15 Changed 9 years ago by
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.pyplot/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 -- ImageMagickI 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 addsage: sympow('-new_data 2') # not testedto 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 9 years ago by
comment:14 Changed 9 years ago by
- Status changed from needs_work to needs_review
comment:15 in reply to: ↑ 13 Changed 9 years ago by
So I believe that both of them should probably be made
optional - precomputations
, or perhaps even better to addsage: sympow('-new_data 2') # not testedto 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: ↓ 17 Changed 9 years ago by
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 9 years ago by
- 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 removedoptional - 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 9 years ago by
But otherwise positive review.
comment:19 follow-up: ↓ 20 Changed 9 years ago by
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 9 years ago by
- 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 9 years ago by
Exactly, but opening a follow up ticket. It's now #14202
comment:22 Changed 9 years ago by
Since the "time" problem has nothing to do with Cygwin, I will keep the
# optional - time
tags.
comment:23 Changed 9 years ago by
- Status changed from positive_review to needs_work
Changed 9 years ago by
comment:24 Changed 9 years ago by
- Status changed from needs_work to needs_review
New version of 14184_manual.patch needs review.
comment:25 Changed 9 years ago by
- Status changed from needs_review to positive_review
Patchbot is confused... apply 14184_optional.patch and 14184_manual.patch
comment:26 Changed 9 years ago by
- Description modified (diff)
- Status changed from positive_review to needs_work
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.
# 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 :-)convert
ones toImageMagick
, or vice versa? Anyway, that should be standardized if it wasn't already elsewhere (this feels familiar)octave
, sometimesOctave
- does capitalization matter? Same withAxiom
.somewhat random
- is that an official designation?< 1/100th of a second
- well, I guess it might be! I guess we can leave that.time
command? The one that we always have? I'm confused here.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 therandom
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.