search

JuliaPy / PyCall.jl

Package to call Python functions from the Julia language
MIT License
1.45k stars 186 forks source link

add 2-arg hash function #1054

Closed jverzani closed 5 months ago

jverzani commented 9 months ago

On discourse (https://discourse.julialang.org/t/sympy-sym-s-as-keys-in-dicts/103995) the issue of using tuples of symbolic keys let to the realization that the 2 argument form of hash resolved to a different code path than the 1-argument form. This consolidates the two.

If this PR is reasonable, it might also make sense to remove the precompile statement for hashsalt and the hashsalt function.

The approach taken in PythonCall might provide an alternative. (Base.hash(x::Py, h::UInt) = reinterpret(UInt, Int(pyhash(x))) - 3h)

codecov-commenter commented 9 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e54c4ee) 67.75% compared to head (3c7a753) 67.78%.

Files Patch % Lines
src/PyCall.jl 66.66% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1054 +/- ## ========================================== + Coverage 67.75% 67.78% +0.03% ========================================== Files 20 20 Lines 2025 2024 -1 ========================================== Hits 1372 1372 + Misses 653 652 -1 ``` | [Flag](https://app.codecov.io/gh/JuliaPy/PyCall.jl/pull/1054/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPy) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/JuliaPy/PyCall.jl/pull/1054/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPy) | `67.78% <66.66%> (+0.03%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPy#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stevengj commented 9 months ago

LGTM. I agree about removing hashsalt.

I can't remember why I did it this way — may be from quite an old Julia version.

jverzani commented 9 months ago

Thanks! Let me know if there is anything else to be done here.

MilesCranmer commented 5 months ago

@stevengj would it be possible to merge this? Thanks! - Miles