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

Update around broadcasting #300

Conversation

hyrodium
Copy link
Contributor

@hyrodium hyrodium commented Mar 21, 2024

Sorry for the late PR.
This PR is related to this comment: #295 (comment).

Changes in this PR:

  • Point(1,2) .+ 3 is now invalid.
    • Use Point(1,2) + Point(3,3) instead.
    • CHANGELOG.md is also updated.
  • [Point(1,2), Point(4,1)] .+ Point(3,4) is now valid.
    • This change is profitable for reducing Ref in code.
      • i.e. we don't need Ref in between.(Ref(pt1), Ref(pt2), range).
      • I have also removed between methods introduced in 90d006b.
    • Historical note
      • On the latest release v3.8.0, [Point(1,2), Point(4,1)] .+ Point(3,4) was valid too.
      • After PR#294, [Point(1,2), Point(4,1)] .+ Point(3,4) turned invalid, and we needed to write [Point(1,2), Point(4,1)] .+ Ref(Point(3,4)) instead.
      • This PR reverts the broadcasting behavior, and [Point(1,2), Point(4,1)] + Point(3,4) is now valid.
      • Of course, [Point(1,2), Point(4,1)] .+ Ref(Point(3,4)) is also still valid.

@cormullion
Copy link
Member

How’s this going?

@hyrodium
Copy link
Contributor Author

hyrodium commented Apr 7, 2024

I'm sorry that I did not have time for OSS in recent days. I will try updating this PR tomorrow! 💪

@hyrodium hyrodium force-pushed the fix/revert_arithmetic_and_broadcasting branch from da9efcb to aa47042 Compare April 8, 2024 16:37
@hyrodium hyrodium marked this pull request as ready for review April 8, 2024 16:49
@hyrodium
Copy link
Contributor Author

hyrodium commented Apr 8, 2024

I think the dictionary order with Point should be also removed before releasing v4.

julia> Point(1,3) < Point(0,4)
false

julia> Point(1,3) < Point(2,1)
true

If you agree with that, I will fix this in another PR!

x-ref: https://github.com/JuliaGraphics/Luxor.jl/blob/master/test/point-arithmetic.jl#L46-L48

@cormullion
Copy link
Member

cormullion commented Apr 8, 2024

Sure! Just let me know when you’ve finished all the changes you feel are important - then I can go ahead and make the 4.0 release.

@hyrodium
Copy link
Contributor Author

This is ready for review!

@cormullion cormullion merged commit fa064b8 into JuliaGraphics:master Apr 15, 2024
5 of 8 checks passed
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.

2 participants