Opened 11 years ago

Closed 11 years ago

#12395 closed defect (fixed)

Change some random variable indexes that cause doctest failures if doctests are run in a different order.

Reported by: David Roe Owned by: Minh Van Nguyen
Priority: minor Milestone: sage-5.0
Component: doctest coverage Keywords:
Cc: Robert Bradshaw Merged in: sage-5.0.beta3
Authors: David Roe Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Texture variables in 3d plotting and some symbolic variables used by maxima have variable names that depend on the order they were created. This patch changes the number to a ...

Attachments (1)

12395.patch (4.4 KB) - added by David Roe 11 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 11 years ago by David Roe

Status: newneeds_review

comment:2 Changed 11 years ago by Karl-Dieter Crisman

I don't like this change for the Maxima. The point is just as much for end users (many of whom will never have seen this type of output) to know what the output should look like as for testing. We've tried to explain what is going on with these; the ... may confuse people who are brand-new but want to solve something. (Also, I think there are more than this out there, not to mention the real and complex constants...)

I understand the point of this patch. I'm just not sure it's necessary, nor advisable in all instances. The textures I have been annoyed by in the past too, and those are relatively internal, so that's a different story.

Also, the commit message is a little long on the first line :)

I'd be interested in hearing from others on these issues, though.

comment:3 Changed 11 years ago by David Roe

The motivation for these particular changes is that Robert and I are rewriting the doctest framework in Sage (in order to clean up the code, use less temp files, perform timing regression testing...), and these tests break in the new framework since code is executed in a slightly different code path.

If you don't think the maxima ones are good changes, we can postpone them to the patch that actually accompanies the new doctesting scripts and just change things to the new numbers.

Once we're agreed on the right approach I'll go back and change the commit message.

comment:4 in reply to:  3 Changed 11 years ago by Karl-Dieter Crisman

Replying to roed:

The motivation for these particular changes is that Robert and I are rewriting the doctest framework in Sage (in order to clean up the code, use less temp files, perform timing regression testing...), and these tests break in the new framework since code is executed in a slightly different code path.

I see. I've heard a little about this - curious as to where the main ticket is for this, though doubtless I wouldn't understand much :) Fewer temp files is certainly a good thing, as is the timing issue - it always seems to bite us.

If you don't think the maxima ones are good changes, we can postpone them to the patch that actually accompanies the new doctesting scripts and just change things to the new numbers.

Probably that's better, at least for now. If we had "dummy numbers" like z123 it might be different, but the z... could look confusing to a newbie, I think.

Once we're agreed on the right approach I'll go back and change the commit message.

Again, I'm surprised there aren't more of the 3d plot guys to change. Maybe these are just the ones whose order changed.

comment:5 Changed 11 years ago by David Roe

Alright, I've postponed the changes to the symbolic stuff. I imagine there are more 3d guys to change, but they don't need to be changed to make the new scripts pass.

comment:6 Changed 11 years ago by Karl-Dieter Crisman

Status: needs_reviewneeds_work

Commit message is now inaccurate, and too long (what is it, 80 characters? I always forget) according to our guidelines... Sorry to be nitpicky.

comment:7 Changed 11 years ago by Robert Bradshaw

See #12415

Changed 11 years ago by David Roe

Attachment: 12395.patch added

comment:8 Changed 11 years ago by David Roe

Status: needs_workneeds_review

I thought I'd changed that. Fixed now. Thanks for the reviews.

comment:9 Changed 11 years ago by Karl-Dieter Crisman

Reviewers: Karl-Dieter Crisman
Status: needs_reviewpositive_review

No problem, looks fine. Testing... passed.

comment:10 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta3
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.