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

geometry: perpendicular segment and orthocenter corrections #1332

Merged
merged 1 commit into from Jun 7, 2012

Conversation

smichr
Copy link
Member

@smichr smichr commented Jun 6, 2012

The perpendicular segment may not terminate on the LinearEntity so
the intersection must be done with the line containing the linear
entity. The same sort of reasoning applies to the orthocenter calculation:
since the altitudes are segments, they may not intersect within the triangle
so lines containing the altitudes should be used to find the intersection.

This addresses issue 3279.

The perpendicular segment may not terminate on the LinearEntity so
the intersection must be done with the line containing the linear
entity. The same sort of reasoning applies to the orthocenter calculation:
since the altitudes are segments, they may not intersect within the triangle
so lines containing the altitudes should be used to find the intersection.
@Krastanov
Copy link
Member

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYvu0aDA

Interpreter: /opt/pym32/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (32-bit)
Cache: yes
Test command: setup.py test
master hash: c157ab3
branch hash: 89665830dcca4f3092db1d9883ce0855d5647970

Automatic review by SymPy Bot.

@Krastanov
Copy link
Member

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY34wbDA

Interpreter: /usr/bin/python2.7 (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: c157ab3
branch hash: fbbc724

Automatic review by SymPy Bot.

@Krastanov
Copy link
Member

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYv-0aDA

Interpreter: /usr/bin/python3 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: c157ab3
branch hash: fbbc724

Automatic review by SymPy Bot.

perpendicular to the opposite side.
An altitude of a triangle is a segment through a vertex,
perpendicular to the opposite side, with length being the
height of the vertex measured from the line containing the side.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original formulation is in correspondence with what Wikipedia says and with my rememberings from geometry. Is there a specific reason you wish to transition to a different definition?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original formulation misuses the word "line" (i.e. infinite line).
A "segment" is the part of the line between two points.

The wikipedia article uses the terms "line" and "segment" correctly,
however the meaning of "altitude" changes from "line" to "segment"
throughout the text.

This method returns the "altitude segment" so I think that the new
docstring is better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Jun 6, 2012 at 4:51 PM, Stefan Krastanov
reply@reply.github.com
wrote:

@@ -1824,8 +1824,9 @@ def is_right(self):
     def altitudes(self):
         """The altitudes of the triangle.

  •        An altitude of a triangle is a straight line through a vertex and
  •        perpendicular to the opposite side.
  •        An altitude of a triangle is a segment through a vertex,
  •        perpendicular to the opposite side, with length being the
  •        height of the vertex measured from the line containing the side.

The original formulation misuses the word "line" (i.e. infinite line).
A "segment" is the part of the line between two points.

The wikipedia article uses the terms "line" and "segment" correctly,
however the meaning of "altitude" changes from "line" to "segment"
throughout the text.

This method returns the "altitude segment" so I think that the new
docstring is better.

Yes, I was trying to make the docstring reflect what the function is
doing, and I agree with Stefan's observation about the wikipedia
article.

Does this read better?

An altitude of a triangle is a segment that is perpendicular to the
line containing a given side of the triangle and passing through the
vertex opposite that side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I haven't read the Wikipedia article to the end, so I didn't know there was abuse of terminology :-( The reason why I commented on this new definition was that, at first sight, it seemed rather unintuitive; I thought I had a better definition in mind, however, when I tried to actually put it down, it turned out to be very convoluted :-(

As for the new version of the definition, I like the first one better ("An altitude of a triangle is a segment through a vertex,..."). The first definition describes one end of the segment, orientation, and length, thus being complete. The second definition describes orientation and one point on the segment, which doesn't look sufficiently unambiguous to me. It is slightly more readable, but I rather wouldn't vote for it.

@smichr
Copy link
Member Author

smichr commented Jun 7, 2012

OK, I'll commit what's there, then. Thanks for the attention in looking this over.

smichr added a commit that referenced this pull request Jun 7, 2012
geometry: perpendicular segment and orthocenter corrections
@smichr smichr merged commit 3920a39 into sympy:master Jun 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants