Opened 12 years ago

Closed 12 years ago

#2859 closed defect (fixed)

[with patch, positive review] arrow3d((0,0,0), (0,0,-1)) plots an *upward* pointing arrow

Reported by: jason Owned by: was
Priority: blocker Milestone: sage-3.0
Component: graphics Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Try the following:

plot(vector((0,0,-1)))

The resulting vector points up, but should point down.

Attachments (1)

trac-2859-arrow3d.patch (2.4 KB) - added by jason 12 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 12 years ago by jason

  • Summary changed from plotting the vector (0,0,-1) really plots (0,0,1) to arrow3d((0,0,0), (0,0,-1)) plots an *upward* pointing arrow

The problem is in arrow3d:

sage: arrow3d((0,0,0), (0,0,-1))
*points up*

comment:2 Changed 12 years ago by jason

All right, this was because we were taking the cross product with (0,0,1) and using that to decide whether or not to rotate the vector, ignoring the case that we point in the opposite direction (but still have a cross product of zero).

Patch will be attached. I'm marking this a blocker since it gives a *wrong* result and William said that wrong results should be marked as blockers.

comment:3 Changed 12 years ago by jason

  • Priority changed from major to blocker

comment:4 Changed 12 years ago by jason

  • Summary changed from arrow3d((0,0,0), (0,0,-1)) plots an *upward* pointing arrow to [with patch, needs review] arrow3d((0,0,0), (0,0,-1)) plots an *upward* pointing arrow

comment:5 Changed 12 years ago by mabshoff

Jason,

how does this ticket related to #1777? [also opened by you]

Cheers,

Michael

comment:6 Changed 12 years ago by jason

It doesn't relate.

In this ticket, the arrow is actually wrong (the assumption in the code is that "parallel to (0,0,1)" == "pointing in the same direction as (0,0,1)", which is just wrong.

The issue in #1777 is a cosmetic issue related to the menus in JMOL.

comment:7 Changed 12 years ago by ddrake

  • Summary changed from [with patch, needs review] arrow3d((0,0,0), (0,0,-1)) plots an *upward* pointing arrow to [with patch, positive review pending] arrow3d((0,0,0), (0,0,-1)) plots an *upward* pointing arrow

The patch works as advertised and the code looks good to me. Positive review for the code, and I'll give it full positive review once you sort out this minor complaint about the doctest: instead of "A downward pointing arrow should have a transformation scaling the points to their negatives" do you mean to say something like "a downward-pointing arrow corresponds to a transformation which reverses the z-coordinates of points"?

Mostly I'm confused because when you give me a vector in R3 and ask me what linear transformation of R3 to itself that it's describing, I naturally think of reflection across the perpendicular plane. The vector (0,0,-1) doesn't say "take (x,y,z) to (-x,-y,-z)" to me...but maybe this is just me, and perhaps it's more an issue with get_transformation than with arrow3d.

At any rate, if you're a bit more clear in that description, you'll be good to go.

comment:8 Changed 12 years ago by jason

Arrows are always constructed pointing up in the z direction from the origin, and then rotated/translated into place. This works for every arrow direction except the -z direction. I take care of the issue by testing to see if the arrow should point in the -z direction, and if it should, just scaling the constructed arrow by -1 (i.e., every point is sent to its negative). The scaled arrow then points downwards. The doctest just tests that the scale of -1 is applied to the arrow. I'm not sure how to make it clearer.

Changed 12 years ago by jason

comment:9 Changed 12 years ago by jason

I put up a new patch with the above explanation above the doctest.

comment:10 Changed 12 years ago by mabshoff

  • Summary changed from [with patch, positive review pending] arrow3d((0,0,0), (0,0,-1)) plots an *upward* pointing arrow to [with patch, positive review] arrow3d((0,0,0), (0,0,-1)) plots an *upward* pointing arrow

Looks good to me. I like the added explanation. Positive review.

Cheers,

Michael

comment:11 Changed 12 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 3.0.alpha3

Note: See TracTickets for help on using tickets.