Opened 4 years ago
Closed 4 years ago
#19302 closed defect (fixed)
Stopgap for Element.__hash__
Reported by:  ncohen  Owned by:  

Priority:  blocker  Milestone:  sageduplicate/invalid/wontfix 
Component:  misc  Keywords:  
Cc:  Merged in:  
Authors:  Reviewers:  Vincent Delecroix  
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
See 72:ticket:19016. Let us warn the users until this bug gets fixed.
Nathann
A discussion about this ticket is currently going on at this sagedevel thread.
Change History (38)
comment:1 Changed 4 years ago by
 Branch set to public/19302
 Commit set to c84d7c704e90b5c762964118af6d06b59b9478fc
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Priority changed from major to blocker
comment:3 followup: ↓ 6 Changed 4 years ago by
I am ok with the stopgap. But I would like more consensus from developers. The most problem I have is that it is very intrusive
sage: G = FreeGroup(2) sage: x,y = G.gens() sage: H = G / [x**2, y**2] /opt/sage/src/bin/sageipython:1: ******************************************************************************** The __hash__ method of Element is broken. Beware of any function that relies on it, as it may return wrong results. This issue is being tracked at http://trac.sagemath.org/sage_trac/ticket/19302. ********************************************************************************
or
sage: p = Polyhedron([(1,1),(2,3),(4,5),(6,7)]) sage: F = p.faces(2) /opt/sage/local/lib/python2.7/sitepackages/sage/graphs/generic_graph.py:19557: ******************************************************************************** The __hash__ method of Element is broken. Beware of any function that relies on it, as it may return wrong results. This issue is being tracked at http://trac.sagemath.org/sage_trac/ticket/19302. ********************************************************************************
... which means that many people will run into that trouble. But "annoying" is better than "wrong". This is why I tend to agree with the proposal.
Another option, would be to move some of the commits from #19016 here (e.g. it would at least avoid the error on polyhedron). At the same time it will decrease the complexity of #19016 which is good. What do you think?
Vincent
comment:4 Changed 4 years ago by
 Commit changed from c84d7c704e90b5c762964118af6d06b59b9478fc to bf004c2def5615b5bfe96338911d00d24f0119ac
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
8082762  Trac 19302: fix infinite polynomial elements

de053ad  Trac 19302: fix similarity classes

30eb651  Trac 19302: fix Kleber tree hash value

18880b8  Trac 19302: fix hash for Additive abelian group elements

e24d2db  Trac 19302: fix fgp vector conversion

418b53e  Trac 19302: hash for polyhedra

019d88f  Trac 19302: change output order in documentation

1a9a678  trac 19302: fix some nondeterministic doctests (and add some deterministic checks)

e74b04e  trac 19302: doctest fixes and making some code deterministic

bf004c2  trac 19302: fix some doctests in polynomial/plural.pyx

comment:5 Changed 4 years ago by
All right. I backported what I could from #19016. It is just a proposition though.
comment:6 in reply to: ↑ 3 ; followup: ↓ 7 Changed 4 years ago by
Hello,
I am ok with the stopgap. But I would like more consensus from developers. The most problem I have is that it is very intrusive
Well, it pops out absolutely everywhere because this bug is very very bad, and can generate wrong results absolutely everywhere.
Another option, would be to move some of the commits from #19016 here (e.g. it would at least avoid the error on polyhedron). At the same time it will decrease the complexity of #19016 which is good. What do you think?
I think that it would make 1000x more sense to create a new ticket entitled "some new hash functions" (or something) and move your commits there rather than here. Splitting #19016 into subtickets does make sense, but I do not see a reason to merge it with this one.
Which you can also flag as 'blocker' if this is what you are after?...
Nathann
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 4 years ago by
Replying to ncohen:
Well, it pops out absolutely everywhere because this bug is very very bad, and can generate wrong results absolutely everywhere.
No, it still needs a confluence of circumstances. It only leads to possibly wrong results for elements where elements that are considered equal can have different string representations. That can happen because generators get renamed (an operation that shouldn't be supported in sage) or because elements are not printed in a canonical form (either because such a form isn't attainable or because it's expensive to attain). There aren't that many parents in sage with that property (and we'll probably have to disallow hashing there). It's worth fixing, but you don't have to overstate the impact.
comment:8 in reply to: ↑ 7 Changed 4 years ago by
It's worth fixing, but you don't have to overstate the impact.
This is a very entertaining opinion. I am glad that I do not have to convince you to change it in order to add this stopgap.
Nathann
comment:9 Changed 4 years ago by
 Commit changed from bf004c2def5615b5bfe96338911d00d24f0119ac to c84d7c704e90b5c762964118af6d06b59b9478fc
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:10 Changed 4 years ago by
Back to where it was. I propose to put #19321 as a dependency. How about that?
Vincent
comment:11 Changed 4 years ago by
Vincent: set #19321 to be a blocker if you think that it should be one.
There is no reason, however, why this oneliner should have a dependency.
Nathann
comment:12 Changed 4 years ago by
Shouldn't the stopgap refer to #19016 (i.e. where the real issue is being tracked)
comment:13 Changed 4 years ago by
 Commit changed from c84d7c704e90b5c762964118af6d06b59b9478fc to 39f4d3cf9c82bc17097eb761e80a4033cc38148f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
39f4d3c  trac #19302: Stopgap for #19016

comment:14 followup: ↓ 15 Changed 4 years ago by
The exact same issue occurs with Mod(3,5) == 8
. Different hashes, compares equal.
comment:15 in reply to: ↑ 14 Changed 4 years ago by
Replying to was:
The exact same issue occurs with
Mod(3,5) == 8
. Different hashes, compares equal.
I think the consensus was that this is a different issue: equality isn't transitive *across different parents*, and hence "equal => equal hash" cannot hold. However, within a parent we do need to try and keep "==" transitive, and we should try to have "equal => equal hash". In cases where we cannot do this, we should make elements unhashable. Via the "_cache_key" method we can still allow such objects to appear as part of cache keys.
I do think that sage 6.9 could still have this flaw unflagged, but we should really try to fix 6.10 significantly in this respect, and progress has shown this is quite attainable (there probably plenty of other bad hashes around but the "hash(str(self))" catchall is definitely a bad offender)
So I'd say: merge the stopgap right after the 6.9 release to ensure we fix this stuff.
comment:16 Changed 4 years ago by
Another ticket I opened a while ago regarding hashes of objects with the same parent:
http://trac.sagemath.org/ticket/15297
I think Nils hits it on the head with regard to not having a default hash (what good is it anyway if you can't reliably use sets containing such objects?), and making some effort to have appropriate hashes within a parent. And I agree that this stopgap is unnecessary, because it has not become a more urgent problem than before.
comment:17 Changed 4 years ago by
My position with the stopgap is pretty simple: if you know that there is a bug in the software that you develop, a bug that may make the user's code return bad answer, *do you tell the users*?
Of course the bug did not appear yesterday, but today we know it exists.
And you don't tell the users? That's irresponsible.
(and, occasionally, contrary to our developer's manual)
Nathann
comment:18 followup: ↓ 20 Changed 4 years ago by
And you don't tell the users? That's irresponsible.
It would be irresponsible if trac were secret. Like the bug trackers that Matlab, Maple, mathematica, and Magma use.
But I know for a fact that Sage has hundreds (maybe thousands) of bugs. That doesn't mean that every time you start Sage it should show a big message about that.
I was the one that came up with the stopgap idea for sage in the first place, so I really like it in most cases.
 William
comment:19 followup: ↓ 21 Changed 4 years ago by
Besides, we *do* tell users:
http://doc.sagemath.org/html/en/developer/coding_in_python.html#thehashspecialmethod
Maybe not in the most easily accessible place, but I feel like I've been aware of this ever since I started using Sage. I would *like* to have consistent hashes for elements with a common parent, but I know I can't count on it.
Anyway: yes, something should be fixed, but no, I don't think it's urgent enough to print disconcerting warnings that make people think Sage is broken.
comment:20 in reply to: ↑ 18 ; followup: ↓ 23 Changed 4 years ago by
It would be irresponsible if trac were secret.
Do you seriously expect them to come here and figure it out?
Or is the whole point of not having a stopgap to *not have them know* and, thus, not worry?
But I know for a fact that Sage has hundreds (maybe thousands) of bugs. That doesn't mean that every time you start Sage it should show a big message about that.
Your code hygiene is a personal taste. All I can say is that this doesn't happen in graphs/, nor in combinat/designs/, nor in the LP code. Seems that we have different expectations.
I was the one that came up with the stopgap idea for sage in the first place, so I really like it in most cases.
To impress me you will have to enforce the idea when it takes guts to do so.
Nathann
comment:21 in reply to: ↑ 19 Changed 4 years ago by
Besides, we *do* tell users:
http://doc.sagemath.org/html/en/developer/coding_in_python.html#thehashspecialmethod
It's funny how we have to explain everything in docstrings or users get it wrong, and on the other hand we expect to figure out hash issues by themselves while browsing the developer's manual. Bullshit.
Maybe not in the most easily accessible place, but I feel like I've been aware of this ever since I started using Sage.
You are a developer. As William for the developers/users ratio.
Anyway: yes, something should be fixed, but no, I don't think it's urgent enough to print disconcerting warnings that make people think Sage is broken.
WTF. Sage *IS* broken. Don't you get it? Having to display a stopgap is a very bad thing, because it worries users, and what we have to do against it is not hide the stopgaps but FIX THE BUGS.
Nathann
comment:22 Changed 4 years ago by
P.S.: I'm not sure I got made my point clear: if you don't like to see a stopgap in Sage the fix is easy: review Vincent's tickets, and help him. That's the only way to deal with this honorably.
comment:23 in reply to: ↑ 20 ; followup: ↓ 27 Changed 4 years ago by
Replying to ncohen:
I was the one that came up with the stopgap idea for sage in the first place, so I really like it in most cases.
To impress me you will have to enforce the idea when it takes guts to do so.
I could care less about impressing you.
I'm 1 to this stopgap in sage6.9. I'm +1 to this being merged at the beginning of sage6.10 and fixed in sage6.10.
comment:24 Changed 4 years ago by
 Description modified (diff)
comment:25 Changed 4 years ago by
Since you want a review from me: 1
Warnings need to be specific. There are lots of elements that have canonical representations, there hashing it is definitely a bit hacky but perfectly valid.
There is definitely room for improvement, but it skirts a bit the line of design choice with hashes and coercion.
Attach the stopgap to finitely presented group elements if you want, or fix it in sage6.10.
comment:26 Changed 4 years ago by
 Milestone changed from sage6.9 to sage6.10
comment:27 in reply to: ↑ 23 ; followups: ↓ 28 ↓ 29 Changed 4 years ago by
Replying to was:
I'm +1 to this being merged at the beginning of sage6.10 and fixed in sage6.10.
You cannot really demand the second part ("fixed in sage6.10"). Basically, at the end of the 6.10 release cycle, you might easily end up in the same situation as now. So any argument to not add the stopgap now is also a valid argument to not add the stopgap in the next release.
I think the logical conclusion (which Volker seems to agree with) is simply to not add this stopgap and close this ticket as wontfix.
comment:28 in reply to: ↑ 27 Changed 4 years ago by
I think the logical conclusion (which Volker seems to agree with) is simply to not add this stopgap and close this ticket as wontfix.
Grow a sense of responsibility. You know what lies ahead? We all get used to return wrong results and do not care anymore.
And you can throw this piece of code to the junk, for nobody will trust it.
Nathann
comment:29 in reply to: ↑ 27 ; followup: ↓ 30 Changed 4 years ago by
Replying to jdemeyer:
Replying to was:
I'm +1 to this being merged at the beginning of sage6.10 and fixed in sage6.10.
You cannot really demand the second part ("fixed in sage6.10"). Basically, at the end of the 6.10 release cycle, you might easily end up in the same situation as now. So any argument to not add the stopgap now is also a valid argument to not add the stopgap in the next release.
I think the logical conclusion (which Volker seems to agree with) is simply to not add this stopgap and close this ticket as wontfix.
+1
Nathann:
Grow a sense of responsibility [...]
Just fix the real problem ASAP instead of complaining.
comment:30 in reply to: ↑ 29 Changed 4 years ago by
Just fix the real problem ASAP instead of complaining.
To quote Jeroen:
You cannot really demand the second part ("fixed in sage6.10").
The only thing you are doing is setting us on the track to failure by weakening our quality procedures.
Nathann
comment:31 Changed 4 years ago by
+1 to keeping this ticket open (with 6.10 milestone).
1 to closing it as wontfix.
comment:32 followup: ↓ 34 Changed 4 years ago by
comment:33 Changed 4 years ago by
+1 to merging this now.
comment:34 in reply to: ↑ 32 ; followup: ↓ 35 Changed 4 years ago by
Replying to nbruin:
Otherwise, we're going to be in the same mess coming 6.10
I predicted this in 27
I think that it really makes no sense to merge this ticket now. Either you think that the broken hash is serious enough to merit a stopgap, or you think it's not serious enough. There is really no way that it can be a serious problem in Sage 6.10, but not a serious problem in Sage 6.9.
comment:35 in reply to: ↑ 34 Changed 4 years ago by
Replying to jdemeyer:
I predicted this in 27
I think that it really makes no sense to merge this ticket now. Either you think that the broken hash is serious enough to merit a stopgap, or you think it's not serious enough. There is really no way that it can be a serious problem in Sage 6.10, but not a serious problem in Sage 6.9.
It makes procedural sense because then people working on development will have a signal and an incentive to do something about relying on the bad hash. We are currently in a situation where we won't ever catch up, because people are adding new classes depending on the default hash at the same rate as we're trying to fix it (see the recently merged sage.combinat.colored_permutations
). Those people should get a signal that they shouldn't be depending on that tobedeprecated hash. The difference is that now we have a good shot at resolving this whole thing before releasing 6.10, whereas this issue received attention too late in the 6.9 cycle for that to be realistic.
comment:36 Changed 4 years ago by
 Milestone changed from sage6.10 to sageduplicate/invalid/wontfix
To celebrate the merger of #19016, which removes the source for this stopgap.
comment:37 Changed 4 years ago by
 Branch public/19302 deleted
 Commit 39f4d3cf9c82bc17097eb761e80a4033cc38148f deleted
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
Yipee!
comment:38 Changed 4 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #19302: Stopgap for #19016