Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
roed314 opened this issue Jan 31, 2012 · 11 comments

Comments

@roed314
Copy link
Contributor

roed314 commented Jan 31, 2012

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

CC: @robertwb

Component: doctest coverage

Author: David Roe

Reviewer: Karl-Dieter Crisman

Merged: sage-5.0.beta3

Issue created by migration from https://trac.sagemath.org/ticket/12395

@kcrisman
Copy link
Member

comment:2

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.

@roed314
Copy link
Contributor Author

roed314 commented Jan 31, 2012

comment:3

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.

@kcrisman
Copy link
Member

comment:4

Replying to @roed314:

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.

@roed314
Copy link
Contributor Author

roed314 commented Feb 2, 2012

comment:5

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.

@kcrisman
Copy link
Member

kcrisman commented Feb 2, 2012

comment:6

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

@robertwb
Copy link
Contributor

robertwb commented Feb 2, 2012

comment:7

See #12415

@roed314
Copy link
Contributor Author

roed314 commented Feb 2, 2012

Attachment: 12395.patch.gz

@roed314
Copy link
Contributor Author

roed314 commented Feb 2, 2012

comment:8

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

@kcrisman
Copy link
Member

kcrisman commented Feb 2, 2012

comment:9

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

@kcrisman
Copy link
Member

kcrisman commented Feb 2, 2012

Reviewer: Karl-Dieter Crisman

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2012

Merged: sage-5.0.beta3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants