Changes between Initial Version and Version 1 of Ticket #19016, comment 99

10/18/15 03:05:44 (5 years ago)


  • Ticket #19016, comment 99

    initial v1  
    11Replying to [comment:97 jdemeyer]:
     2> 1. In `src/sage/rings/polynomial/laurent_polynomial.pyx`, the function `__hash__` is defined twice in the same class.
     4Can you give line numbers? I count only two definitions of `__hash__` in
     5`src/sage/rings/polynomial/laurent_polynomial.pyx`, and they are on distinct classes:
     12> 2. This isn't proper doctest syntax:
     14Thanks; should be fixed.
     16> 3. Could you justify the need to add lots of comparison functions?
     18Yes, I wonder too. Not including those would save us adding a whole bunch of silly doctests too.
     20> 4. Why is this needed?
     21> {{{
     22> #!diff
     23> -        return self.element_class(self, m)
     24> +        return self.element_class(self, ZZ(m))
     25> }}}
     27If `self.m` should be an `Integer` this is a good way of enforcing it. I don't think we did this before? perhaps this is covered in `element_class` already. I'm not familiar with this code (and didn't add that patch)
    229> 5. This looks very suspicious:
    330> {{{
    1643> }}}
    1744See comment:86 and #19388.
     46> 6. Do you really need to add the constant-zero hash to the various ideal classes? Isn't it sufficient to add it to the base class `Ideal_generic`?
     48I think it's better in the long run for sage if we don't. Having a 0 hash is really bad, but not so easy to diagnose quickly. If it really is too difficult to normalize ideal generators in such a way that one can derive a reliable hash, it's probably better if those ideals are not hashable and hence are not used as dict keys. I think the present "0" hashes are just put in so that we don't break legacy code. Future ring implementations should pay more attention to if and how their ideals can be hashed. The problem is more visible if there is no hash available for ideals by default.