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

Implement equality for Line #105

Closed
juliohm opened this issue Mar 30, 2021 · 19 comments · Fixed by #119
Closed

Implement equality for Line #105

juliohm opened this issue Mar 30, 2021 · 19 comments · Fixed by #119
Labels
feature good first issue Good for newcomers

Comments

@juliohm
Copy link
Member

juliohm commented Mar 30, 2021

We need a method l1 == l2 that tells when two lines l1 and l2 are equal.

@juliohm juliohm added good first issue Good for newcomers feature labels Mar 30, 2021
@hyrodium
Copy link
Contributor

hyrodium commented Apr 12, 2021

I'm trying to solve this issue, and I have one question.

Does a line have a parameter or a direction?

  • If have a parameter: Line(p1, p2) == Line(p1, p2) holds. (in this case, we don't need additional implimentations.)
  • Or, if have a just direction: Line(p1, p2) == Line(p1, p1+2*(p2-p1)) holds.
  • Or, if don't have both: Line(p1, p2) == Line(p1+2*(p2-p1), p1) holds.

@juliohm
Copy link
Member Author

juliohm commented Apr 12, 2021

Thank you @hyrodium , did you read the docstring already? What is not clear? If I remember correctly our current implementation uses two points on the line to define it.

@hyrodium
Copy link
Contributor

hyrodium commented Apr 12, 2021

Sorry for my confusing English.

Yes, I've already read the docstring of Line, but I was misreading:bow:
My updated question is, "Do we need additional implementation?"

It seems like the following properties are already satisfied.

julia> Line(Point(1,2), Point(2,3)) == Line(Point(1,2), Point(2,3))
true

julia> Line(Point(1,2), Point(2,3)) == Line(Point(1,2), Point(2,2))
false

sorry if I misunderstood something.


At first, I thought Line(Point(1,2), Point(2,4)) == Line(Point(1,2), Point(3,6)) should be satisfied and we have to implement Base.== for this, because these lines are identical as sets on Eucledian space.
But, I found a method (l::Line)(t), so the line has parameter and directions.

@juliohm
Copy link
Member Author

juliohm commented Apr 12, 2021

We are aiming for equality of lines so even though two lines may be specified with different pairs of points they may be the same line. Makes sense? That is what is not implemented yet and would be a good contribution for a first-time contributor.

@hyrodium
Copy link
Contributor

Let me use an example to confirm this.
Do you think, the following l1 == l2 should be true?

l1 = Line(Point(1,2), Point(2,4))
l2 = Line(Point(1,2), Point(3,6))
l1 == l2

image

But, the lines l1 and l2 are different as map, so I thought l1 ≠ l2 should be true.

julia> l1(1)
Point(2, 4)

julia> l2(1)
Point(3, 6)

@juliohm
Copy link
Member Author

juliohm commented Apr 12, 2021

The parameter t can vary from -inf to +inf so they are the same line.

@hyrodium
Copy link
Contributor

Okay, I understood.
Maybe it's better to add docstring to (l::Line)(t) like this:

"""
Note that same lines may have different parameters.

```jldoctest
julia> l1 = Line(Point(1,2), Point(2,4));

julia> l2 = Line(Point(1,2), Point(3,6));

julia> l1 == l2
true

julia> l1(1), l2(1)
(Point(2, 4), Point(3, 6))
```
"""
(l::Line)(t) = l.a + t * (l.b - l.a)

@hyrodium
Copy link
Contributor

It seems like that we also need equality for Segment.

julia> s1 = Segment(Point(1,2), Point(3,6));

julia> s2 = Segment(Point(3,6), Point(1,2));

julia> s1 == s2
false

@serenity4
Copy link
Member

Indeed l1 and l2 represent the same set. Their parametrization is however different as you pointed out. I think this Line type aims to represent sets spanned by undirected lines. Under these terms, they are equal, even if instances are not identical. Therefore I would recommend implementing an equality operator == that checks whether both lines represent the same set, and an identity operator === that checks whether the parametrization is the same (which is true only if the points are equal).

@serenity4
Copy link
Member

Segment is, however, an oriented line segment, so s1 and s2 should not be equal. This makes it a bit weird since a Line is not simply the continuation of a Segment, differing by the fact that a Line is not oriented.

@hyrodium
Copy link
Contributor

hyrodium commented Apr 12, 2021

Therefore I would recommend implementing an equality operator == that checks whether both lines represent the same set, and an identity operator === that checks whether the parametrization is the same (which is true only if the points are equal).

Okay, but, how can I check these inequality?

l1 = Line(Point(0.0, 0.0), Point(1.0, 2.0))
l2 = Line(Point(0, 0), Point(1, 2))
  • == is for checking as set
  • === is for checking as map

Is there any other equatility symbol?
I thought we need some operator that check as map, but not type.

@hyrodium
Copy link
Contributor

And also, I suspect the checking equality as set will be hard because of floating number, and the transitivity of == would be broken.
Should we use isapprox method instead?

(Sorry for many questions, someone can solve this issue smarter than me:wink:)

@serenity4
Copy link
Member

Hmm, indeed if we overload === to check for equality between the parametrization (points) of lines, then we'd be missing an operator to check for true identity (in the sense that types and contents are the same). Might also break some code that rely on === to be related to the true identity. I think we do want to have three different operators. I don't have a solution in mind at the moment though.

@serenity4
Copy link
Member

isapprox looks very much related to == to me (in that it only adds tolerances), so using it as the "third" operator to implement something semantically different than == doesn't seem like a good solution to me.

@hyrodium
Copy link
Contributor

hyrodium commented Apr 13, 2021

isapprox looks very much related to == to me (in that it only adds tolerances)

I really agree with that.

How about adding methods equalasset(A,B) and ∼(A,B) = equalasset(A,B)(\sim) for equality as a set? (Another option is (\simeq).)

  • (or ) equality as a set (apploximately)
  • equality as a map (apploximately)
  • == equality as a map
  • === equality as a true identity, i.e. as a map including types

@juliohm
Copy link
Member Author

juliohm commented Apr 13, 2021

Let't keep it simple @hyrodium , it is the best approach to start here. Simply define == to mean equality as described above regardless of parametrization and implement this equality with a given tolerance using isapprox like we do in other places. We have atol(T) in the library for that.

@hyrodium
Copy link
Contributor

Thanks for the suggestion.
Yes, I love simplicity, but I think sometimes it can't be.

Sorry for repeating, but there're two reasons for my concern.

  • == should have transitivity (i.e. a==b and b==c, then a==c).
    • But, it can't be because of floating number.
    • Julia language really care about the transitivity of ==. (example1, example2)
  • Equal objects should behave equally, including mapping.
    • If l1==l2, then l1(1)==l2(1) should hold.
    • This aim is necessary for explicit programming, rather than set theory.

Do you have any comments on my concern?

I'm really new to this package, and maybe I haven't understood the concept of this package well, but I think the definition of equality is really important for everyone.

(Changing the equal sign makes easy to understand, I guess. That would be one of our choices.)

@juliohm
Copy link
Member Author

juliohm commented Apr 13, 2021

@hyrodium what I am trying to say is that this issue of sorting out a transitive == seems much deeper than what we had in mind originally for this specific issue where we are sharing thoughts. The discussion around == is super important but it touches the entire codebase, and you would need a massive pull request to fix.

Just helping with an approximate == for lines (even if it is not transitive in some corner cases) would be a great start. Then you could open a separate issue to pin point where in the code base we should consider switching to a different symbol for the operation.

@hyrodium
Copy link
Contributor

Ah, Okay, I understoold.
I'll make a PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants