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

added ⋆ as unary operator #31604

Merged
merged 5 commits into from Apr 9, 2019

Conversation

@chakravala
Copy link
Contributor

commented Apr 3, 2019

as disccussed in star is not an operator

chakravala added 2 commits Apr 3, 2019
@MasonProtter

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Can it be a unary-and-binary-op instead? The Moyal Star product is a legit use of this operator. I don't feel strongly about this though, so if there's a good argument to not bloat the list of unary-and-binary-ops feel free to ignore me.

@chakravala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@MasonProtter good point, I just committed the change

@chakravala chakravala force-pushed the chakravala:patch-1 branch from b86d3c8 to 500a2b2 Apr 4, 2019

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

For this to work as an infix operator it also needs to be added to a precedence level at the top of the file.

@JeffBezanson JeffBezanson added the parser label Apr 4, 2019

@chakravala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

the prec-times precedence is probably the most appropriate, given that Moyal product is a "product", so that's what I have committed for now

@chakravala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

any update on whether could still be merged before v1.2 is frozen?

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

So you want the unary operator version of this to be prefix not postfix? Isn't the traditional Kleene star notation postfix?

@improbable22

This comment has been minimized.

Copy link

commented Apr 4, 2019

This PR seems to be about \star which was already a binary operator, and is now listed twice in prec-times.

But the discourse thread was mostly about \bigstar ★. Which would also be nice to have.

@chakravala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Yes, prefix (not postfix) is used for the Hodge star operation that I implemented in Grassmann.jl

julia> using Grassmann

julia> @basis^3
(⟨+++⟩, v, v₁, v₂, v₃, v₁₂, v₁₃, v₂₃, v₁₂₃)

julia> ⋆(v1), ⋆(v2), ⋆(v3)
(v₂₃, -1v₁₃, v₁₂)

@stevengj recommended to use \star instead of \bigstar in the discourse

@MasonProtter

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@improbable22 is right, \star is already a binary operator. My bad.

julia> ⋆(x,y) = (x+y)/(x - y)
⋆ (generic function with 1 method)

julia> 12
-3.0
@chakravala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

So it should not be added to unary-and-binary-ops and prec-times; while unary-ops is sufficient?

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Oh, sorry I misunderstood --- the linked discourse post is about adding \bigstar, so I assumed this was adding that new operator.

src/julia-parser.scm Outdated Show resolved Hide resolved

@chakravala chakravala force-pushed the chakravala:patch-1 branch from 0f037a0 to 5efcf0b Apr 4, 2019

@chakravala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

alright, I have reverted the last 2 commits then

@stevengj

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

I think both and .⋆ still need to be added to unary-and-binary-ops (in addition to unary-ops).

@chakravala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

are you sure? because this does work for me without it

julia> ⋆(x) = x
⋆ (generic function with 1 method)

julia>8
8

julia> ⋆(x,y) = x+y
⋆ (generic function with 2 methods)

julia> 78
15

I also get some compile errors when I have unary-and-binary-ops also, so I think it's not needed

Sysimage built. Summary:
Total ───────  83.465780 seconds 
Base: ───────  27.608859 seconds 33.0781%
Stdlibs: ────  55.854563 seconds 66.9191%
    JULIA usr/lib/julia/sys-o.a
Internal error: encountered unexpected error in runtime:
UndefVarError(var=:li)
rec_backtrace at /home/flow/julia-char/julia/src/stackwalk.c:94
jl_throw at /home/flow/julia-char/julia/src/task.c:210
jl_undefined_var_error at /home/flow/julia-char/julia/src/rtutils.c:130
jl_get_binding_or_error at /home/flow/julia-char/julia/src/module.c:290
_uncompressed_ast at ./reflection.jl:906
typeinf_ext at ./compiler/typeinfer.jl:560
typeinf_ext at ./compiler/typeinfer.jl:599
unknown function (ip: 0x7fc1eb33e417)
jl_apply_generic at /home/flow/julia-char/julia/src/gf.c:2191
jl_apply at /home/flow/julia-char/julia/src/julia.h:1604 [inlined]
jl_type_infer at /home/flow/julia-char/julia/src/gf.c:207
jl_compile_method_internal at /home/flow/julia-char/julia/src/gf.c:1773
jl_apply_generic at /home/flow/julia-char/julia/src/gf.c:2196
jl_apply at /home/flow/julia-char/julia/src/julia.h:1604 [inlined]
jl_finish_task at /home/flow/julia-char/julia/src/task.c:167
jl_threadfun at /home/flow/julia-char/julia/src/partr.c:217
start_thread at /usr/lib/libpthread.so.0 (unknown line)
clone at /usr/lib/libc.so.6 (unknown line)
Generating precompile statements...

this error doesn't happen when unary-and-binary-ops is not used

this PR should be okay as-is right now

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

I also get some compile errors when I have unary-and-binary-ops also, so I think it's not needed

That error is definitely not related to this (fix: #31613).

Try [1 ⋆2] and [1 ⋆ 2]; I think those cases are affected by it.

@chakravala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Alright, here is before

julia> [12], [12]
([3], [3])

and after

julia> [12], [12]
([1 2], [3])

you are right, this case needs to be resolved, so I have added it to unary-and-binary-ops again

@chakravala chakravala force-pushed the chakravala:patch-1 branch from bc807bb to 9395215 Apr 4, 2019

@chakravala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

any more comments? is this PR approved?

src/julia-parser.scm Outdated Show resolved Hide resolved
@chakravala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

any more comments on the star operation? hoping to have this merged for v1.2

@StefanKarpinski StefanKarpinski merged commit 6e33c95 into JuliaLang:master Apr 9, 2019

13 of 14 checks passed

buildbot/tester_freebsd64 Run complete
Details
buildbot/package_freebsd64 Run complete
Details
buildbot/package_linux32 Run complete
Details
buildbot/package_linux64 Run complete
Details
buildbot/package_macos64 Run complete
Details
buildbot/package_win32 Run complete
Details
buildbot/package_win64 Run complete
Details
buildbot/tester_linux32 Run complete
Details
buildbot/tester_linux64 Run complete
Details
buildbot/tester_macos64 Run complete
Details
buildbot/tester_win32 Run complete
Details
buildbot/tester_win64 Run complete
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
chakravala added a commit to chakravala/julia-vim that referenced this pull request Apr 19, 2019
carlobaldassi added a commit to JuliaEditorSupport/julia-vim that referenced this pull request Apr 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.