Opened 4 years ago

Closed 4 years ago

#19302 closed defect (fixed)

Stopgap for Element.__hash__

Reported by: ncohen Owned by:
Priority: blocker Milestone: sage-duplicate/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 vdelecroix)

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 sage-devel thread.

See also: #19321, #19331

Change History (38)

comment:1 Changed 4 years ago by ncohen

  • Branch set to public/19302
  • Commit set to c84d7c704e90b5c762964118af6d06b59b9478fc
  • Status changed from new to needs_review

New commits:

c84d7c7trac #19302: Stopgap for #19016

comment:2 Changed 4 years ago by ncohen

  • Priority changed from major to blocker

comment:3 follow-up: Changed 4 years ago by vdelecroix

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/sage-ipython: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/site-packages/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

Last edited 4 years ago by vdelecroix (previous) (diff)

comment:4 Changed 4 years ago by git

  • Commit changed from c84d7c704e90b5c762964118af6d06b59b9478fc to bf004c2def5615b5bfe96338911d00d24f0119ac

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

8082762Trac 19302: fix infinite polynomial elements
de053adTrac 19302: fix similarity classes
30eb651Trac 19302: fix Kleber tree hash value
18880b8Trac 19302: fix hash for Additive abelian group elements
e24d2dbTrac 19302: fix fgp vector conversion
418b53eTrac 19302: hash for polyhedra
019d88fTrac 19302: change output order in documentation
1a9a678trac 19302: fix some non-deterministic doctests (and add some deterministic checks)
e74b04etrac 19302: doctest fixes and making some code deterministic
bf004c2trac 19302: fix some doctests in polynomial/plural.pyx

comment:5 Changed 4 years ago by vdelecroix

All right. I backported what I could from #19016. It is just a proposition though.

comment:6 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by ncohen

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 ; follow-up: Changed 4 years ago by nbruin

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 ncohen

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 git

  • 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 vdelecroix

Back to where it was. I propose to put #19321 as a dependency. How about that?

Vincent

comment:11 Changed 4 years ago by ncohen

Vincent: set #19321 to be a blocker if you think that it should be one.

There is no reason, however, why this one-liner should have a dependency.

Nathann

comment:12 Changed 4 years ago by jdemeyer

Shouldn't the stopgap refer to #19016 (i.e. where the real issue is being tracked)

comment:13 Changed 4 years ago by git

  • Commit changed from c84d7c704e90b5c762964118af6d06b59b9478fc to 39f4d3cf9c82bc17097eb761e80a4033cc38148f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

39f4d3ctrac #19302: Stopgap for #19016

comment:14 follow-up: Changed 4 years ago by was

The exact same issue occurs with Mod(3,5) == 8. Different hashes, compares equal.

https://cloud.sagemath.com/projects/4a5f0542-5873-4eed-a85c-a18c706e8bcd/files/support/2015-10-01-081910-hash.sagews

comment:15 in reply to: ↑ 14 Changed 4 years ago by nbruin

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 Stefan

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 ncohen

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 follow-up: Changed 4 years ago by was

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 follow-up: Changed 4 years ago by Stefan

Besides, we *do* tell users:

http://doc.sagemath.org/html/en/developer/coding_in_python.html#the-hash-special-method

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 ; follow-up: Changed 4 years ago by ncohen

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 ncohen

Besides, we *do* tell users:

http://doc.sagemath.org/html/en/developer/coding_in_python.html#the-hash-special-method

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 ncohen

P.S.: I'm not sure I 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.

Last edited 4 years ago by ncohen (previous) (diff)

comment:23 in reply to: ↑ 20 ; follow-up: Changed 4 years ago by was

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'm stating facts about. I could care less about impressing you.

I'm -1 to this stopgap in sage-6.9. I'm +1 to this being merged at the beginning of sage-6.10 and fixed in sage-6.10.

Version 0, edited 4 years ago by was (next)

comment:24 Changed 4 years ago by vdelecroix

  • Description modified (diff)

comment:25 Changed 4 years ago by vbraun

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 sage-6.10.

comment:26 Changed 4 years ago by vbraun

  • Milestone changed from sage-6.9 to sage-6.10

comment:27 in reply to: ↑ 23 ; follow-ups: Changed 4 years ago by jdemeyer

Replying to was:

I'm +1 to this being merged at the beginning of sage-6.10 and fixed in sage-6.10.

You cannot really demand the second part ("fixed in sage-6.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 ncohen

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 ; follow-up: Changed 4 years ago by was

Replying to jdemeyer:

Replying to was:

I'm +1 to this being merged at the beginning of sage-6.10 and fixed in sage-6.10.

You cannot really demand the second part ("fixed in sage-6.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 ncohen

Just fix the real problem ASAP instead of complaining.

To quote Jeroen:

You cannot really demand the second part ("fixed in sage-6.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 dimpase

+1 to keeping this ticket open (with 6.10 milestone).

-1 to closing it as wontfix.

comment:32 follow-up: Changed 4 years ago by nbruin

Can we just merge this now or merge #19016 now? Otherwise, we're going to be in the same mess coming 6.10, when merging #19016 is bad because of untested consequences.

comment:33 Changed 4 years ago by Stefan

+1 to merging this now.

comment:34 in reply to: ↑ 32 ; follow-up: Changed 4 years ago by jdemeyer

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 nbruin

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 to-be-deprecated 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 nbruin

  • Milestone changed from sage-6.10 to sage-duplicate/invalid/wontfix

To celebrate the merger of #19016, which removes the source for this stopgap.

comment:37 Changed 4 years ago by vdelecroix

  • Authors Nathann Cohen deleted
  • 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 vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.