Skip to content
This repository

number traits should cast if value doesn't change #942

Closed
minrk opened this Issue October 29, 2011 · 13 comments

4 participants

Min RK Fernando Perez Robert Kern Thomas Kluyver
Min RK
Owner

The most common trait error I see is integers failing because they were passed a long, thanks to json or some such.

The non-casting number traits should try casting, and if the value does not change let it go.

So if you have:

class C(HasTraits):
    a = Int()
    b = Float()

The following should work:

c = C()
c.a = 4
c.a = 4L
c.a = 4.0
c.b = 4
c.b = 4L
c.b = 4.0

But still prevent the following:

c = C()
c.a = 4444444444444444444444444444444444444444444444L
c.a = 4.1

I'm less concerned about float <=> int conversion, but the long/int conversion should really be allowed, as it happens a lot and is usually due to annoying behavior in libraries that don't support multiple integer types.

This would break traits compatibility, which also has this unpleasant behavior. If we want to preserve traits compat, then we should just replace all Int/Long instances with their casting counterparts, because this gets annoying.

Fernando Perez
Owner

My vote is to preserve traits semantics as far as we can. I have in my radar interactive widget capabilities, and eventually we may be able use real Traits for that, so I'd suggest for now we don't break compatibility. We will do it if we must, but in this case it seems to me that just using casting Traits explicitly is a simple solution.

Min RK
Owner

Yeah, it is a simple solution - the difference is that regular casting does not require the value to be preserved - e.g. there are some situations where CInt(0.5) should probably raise, as 0 may not be acceptable, nor was it actually specified.

But it also means that we should never use non-casting traits (Int at least), since they suffer from this silly error.

The smallest possible change is to allow ints given as unnecessary longs, and not do any other casting. This would protect us from forward-looking libraries thinking of python 3, where all ints are longs. Real traits do raise on this, but I would honestly consider that a bug in traits, as I consider it a bug in traitlets.

Fernando Perez
Owner
  • In your example, if 0 isn't acceptable for a CInt variable, then I think that should be the job of the trait validator associated with that variable to raise. The casting behavior is to do 0.5 -> 0, so whether 0 is OK or not is something that variable should determine by itself.

  • I think some of this should go into the dev guide, so it's written up somewhere for future reference.

Fernando Perez
Owner

One more thing: we can always take up the discussion with Enthought-dev regarding the design and py3 compatibility. Traits can evolve, after all. I just don't want to deviate without both a very clear reason and at least an attempt at maintaining compatibility with them.

Min RK
Owner
Fernando Perez
Owner

How do you want to proceed on this one? One option is to bounce the discussion over to Enthought-dev on the basic design intent. They've written far more traits code than we ever will, so they must have run into these issues plenty of times...

Min RK
Owner

I suppose we should ping enthought-dev. I do retract my suggestion for general non-altering casts, and I think we should just allow int<=>long to be transparent, as nobody actually uses longs for small integers on purpose, but it does happen a lot by accident from various libraries.

Robert Kern
Collaborator

We could introduce (both traits and traitlets) an Integer trait that accepts both int and long values without alteration.

I do recommend against using the CInt, etc. indiscriminately because I have observed that they do lead to errors where you coerce things to incorrect values without noticing it.

Min RK
Owner

Maybe Integer is the right answer, but it just seems so unnecessary.

Are there any cases where turning unnecessary longs into ints is not the right thing to do?

Why should A.a = 4L should raise, when there is exactly no ambiguity about what should happen. The error message The 'a' trait of an A instance must be an integer, but a value of 4L <type 'long'> was specified is just silly - you asked for an integer, I gave you an integer (perhaps this is why the int/long distinction is removed in Python 3).

This is actually more strict than C regarding assigning values from larger to smaller integer types, and it doesn't make a lot of sense to me that Python code should be more strict about types than C, especially when the fix is trivial as it is here.

Robert Kern
Collaborator

There was a "types are awesome" cabal when Traits's semantics were being laid out. I'm in your camp as to the desired behavior. I just quail at fixing the existing documentation and the tests to match them. Adding a new trait with the desired semantics is much easier.

Min RK
Owner

Fair enough - should Integer downcast small longs to ints? I can see that being handy, as a layer protecting code that doesn't handle longs properly (e.g. doing a naive isinstance(foo, int) check).

Thomas Kluyver
Collaborator

I ran into this when testing PyPy support. Integers come back from an SQLite database as longs there, so I had to switch an Int to a CInt.

Fernando Perez
Owner

Closed by 293d3ee, should have autoclosed.

Fernando Perez fperez closed this November 19, 2011
Stefan van der Walt stefanv referenced this issue from a commit in stefanv/ipython November 01, 2011
Min RK add Integer traitlet
Most int traits are now Integers

Integer differs from Long only in that small `long`s are cast to `int`, rather than
all `int`s being cast to `long`:

    Integer(4L) => 4
    Long(4) => 4L

closes gh-942, closes gh-996.

Rebased to avoid recursive merge for just one commit.
293d3ee
Fernando Perez fperez referenced this issue from a commit January 10, 2012
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.