Opened 8 years ago

Closed 8 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: roed Owned by: mvngu
Priority: minor Milestone: sage-5.0
Component: doctest coverage Keywords:
Cc: robertwb Merged in: sage-5.0.beta3
Authors: David Roe Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

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 roed 8 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by roed

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by kcrisman

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 follow-up: Changed 8 years ago by 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.

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 8 years ago by kcrisman

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 8 years ago by roed

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 8 years ago by kcrisman

  • Status changed from needs_review to needs_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 8 years ago by robertwb

See #12415

Changed 8 years ago by roed

comment:8 Changed 8 years ago by roed

  • Status changed from needs_work to needs_review

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

comment:9 Changed 8 years ago by kcrisman

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

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

comment:10 Changed 8 years ago by jdemeyer

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