TimeDelta

[philtay]

#105

[philtay]

The Travis failure is not caused by my code. Can I increase to 110 the line length? It's better.

[sloria]

Can you please add a test that reaches this branch?

[sloria]

Looks good. Just a couple more tests, and this should be good to merge.

[philtay]

The test_invalid_timedelta_field_deserialization test already reaches the branches you indicated.

# ValueError
int('')

# TypeError
int([])

# OverflowError
field = fields.TimeDelta(fields.TimeDelta.DAYS)
field.deserialize(9999999999)

[sloria]

Right, those branches are reached; could you also make assertions about the error messages?

e.g.

        with pytest.raises(UnmarshallingError) as excinfo:
            field.deserialize(in_value)
        expected = ('{0!r} cannot be interpreted as '
                    'a valid period of time.').format(in_value)
        assert expected in str(excinfo)

[philtay]

Done

[sloria]

I wonder if we can save unneeded computation here by only computing to the requested precision. Something like:

    def _serialize(self, value, attr, obj):
        try:
            days = value.days
            if self.precision == self.DAYS:
                return days
            else:
                seconds = days * 86400 + value.seconds
                if self.precision == self.SECONDS:
                    return seconds
                else:  # microseconds
                    return seconds * 10**6 + value.microseconds
        except AttributeError:
            msg = '{0!r} cannot be formatted as a timedelta.'.format()
            raise MarshallingError(getattr(self, 'error', None) or msg)

Perhaps there is a more elegant way, but it would be good remove the extra computation.

[philtay]

I thought about it. The CPU cycles waste is really negligible (integer math). Saving them would mean uglier code, like the big try block or a lot of if-else. It doesn't worth it.

[philtay]

And I'd be curious to benchmark them: full C speed integer math vs big try and nested if-else. Who will win? :)

[sloria]

Here's a quick-and-dirty comparison of serializing a bunch of timedeltas to microsecond values: http://nbviewer.ipython.org/gist/sloria/5c1b845a6ec59c2404d0

[philtay]

LOL, you really have benchmarked them! However, this confirms that we don't have to worry about performance in this case and that we can avoid ugly code. The difference is negligible and probably within statistical error. Moreover, considering that you have tested with precision='microseconds', the math involved in both TimeDelta1 and TimeDelta2 is exactly the same. Thus the difference in performance can't be attributed to multiplications and additions.

[philtay]

You should use precision='days' to get the real difference and then subtract 0.64 sec (5.99 - 5.35). Anyway, if you really prefer the try-if-else approach, please feel free to uglify the code yourself after merge ;)

[sloria]

Here are comparisons with each level of precision: http://nbviewer.ipython.org/gist/sloria/6229107fa5cea79c80a2. The try-if-else implementation is consistently faster. As expected, the difference is the largest with days.

If you don't want to make the change, I'll go ahead and make it when I merge.

[philtay]

ok, make the change when you merge