-
Notifications
You must be signed in to change notification settings - Fork 33
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
add trivial hash function for spoly #121
Conversation
bde44c7
to
9f7229e
Compare
Codecov Report
@@ Coverage Diff @@
## master #121 +/- ##
=====================================
Coverage 0% 0%
=====================================
Files 30 30
Lines 2590 2592 +2
=====================================
- Misses 2590 2592 +2
Continue to review full report at Codecov.
|
|
||
R, (x, y) = PolynomialRing(QQ, ["x", "y"]) | ||
|
||
@test unique([x, x+y-y]) == [x] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test will occasionally pass even without overloaded hash function;
we should add test which directly compares hashes
in this case these should be the same (and we don't have negative example due to trivial definition above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 1bb108f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is test_spoly_unique
a useful test at all? It only tests one particular symptom, not the core issue, which is tested in test_spoly_hash
. Is there another reason to test unique
on polynomials like this?
Why don't you implement a hash for polynomials using the example of the hash for MPoly's, but just change the random UInt that is used there, say use 0xf0b2b1cdda4bf0b8. Then implement hash for n_Z and n_Q in a similar way to how it is implemented for fmpz and fmpq in Nemo, again with a fixed random UInt in each case (you can get one from rand(UInt)), then implement a "trivial" hash for the other number types, though you could do much better than trivial by at least xoring with the hash of the characteristic and any other information you can get your hands on. This would take about 20 minutes to do and would be infinitely better than a trivial hash. |
Then perhaps open a ticket and let @hannes14 know that we need a way of getting an integer value out of an n_Zn and other number types to be used in the hash functions, so that the hash issue in Singular.jl is finally resolved, instead of remaining in its current state indefinitely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe squash your changes into one commit? Maybe drop the unique
test (unless you really think it is useful)?
|
||
R, (x, y) = PolynomialRing(QQ, ["x", "y"]) | ||
|
||
@test unique([x, x+y-y]) == [x] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is test_spoly_unique
a useful test at all? It only tests one particular symptom, not the core issue, which is tested in test_spoly_hash
. Is there another reason to test unique
on polynomials like this?
For the case that the behavior of |
Yes, I am fine with this once we agree that this can be merged. |
src/poly/poly.jl
Outdated
@@ -278,7 +278,7 @@ end | |||
############################################################################### | |||
|
|||
function Base.hash(p::spoly{T}, h::UInt) where T <: Nemo.RingElem | |||
return h # FIXME: Implement a useful hash function instead | |||
return total_degree(p) * h # FIXME: Implement a more useful hash function instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since Base.hash
by default sets h=UInt(0)
, it's not really much better than just h
;-)
I have no idea what are the inners of spoly
, but a simple implementation could be
hash(p::spoly{T}, h::UInt) where T = xor(hash(spoly), hash(coefficients_or_something_unique_to(p), h))
hash(spoly)
can be of course precomputed first (add a comment).
Or if You can iterate over coefficients of p
then
hash(p::spoly, h::UInt) = xor(HASH_SPOLY, reduce(xor, hash(c, h) for c in coeffs(p))
But I'd really like to see it as UInt(0)
globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't make the hash depend on the object pointer at all. You have to use the data in the actual polynomial, i.e. the coefficients.
For about the fifth time, this is not difficult to do in the slightest and we have examples of how to do it in Nemo and AbstractAlgebra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until there is a better implementation, could you please reconsider this PR? I consider #120 a serious issue. |
This is superceded by PR 139. |
This aims to address #120.