beam-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Valentyn Tymofieiev (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (BEAM-5255) Fix over-aggressive division futurization.
Date Wed, 26 Sep 2018 20:59:00 GMT

    [ https://issues.apache.org/jira/browse/BEAM-5255?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16629399#comment-16629399
] 

Valentyn Tymofieiev commented on BEAM-5255:
-------------------------------------------

I did an audit of all  initial futurization  PRs from that series, looking for  incorrect
introduction of integer division (using //) for incorrect implicit introduction of float division
(via from __future__ import division).

I did not find any regressions besides a regression in microbenchmarks, which is now fixed.


There was one suspicious place, see the lines below, where the underlying types of  arguments
of division are  not obvious, and we are forcing float division where previously an integer
division may have been used, for example if  SourceBundle weights are integers. 

https://github.com/apache/beam/blob/6f6feaaeebfc82302ba83c52d087b06a12a5b119/sdks/python/apache_beam/io/concat_source.py#L142
https://github.com/apache/beam/blob/6f6feaaeebfc82302ba83c52d087b06a12a5b119/sdks/python/apache_beam/io/concat_source.py#L212
https://github.com/apache/beam/blob/6f6feaaeebfc82302ba83c52d087b06a12a5b119/sdks/python/apache_beam/io/concat_source.py#L254

As far as I can tell, we are still doing the right thing, since the goal of that computation
is ultimately to compute fraction of consumed positions in the source, which should be a real
number between 0.0 and 1.0.


> Fix over-aggressive division futurization.
> ------------------------------------------
>
>                 Key: BEAM-5255
>                 URL: https://issues.apache.org/jira/browse/BEAM-5255
>             Project: Beam
>          Issue Type: Bug
>          Components: sdk-py-core
>    Affects Versions: 2.6.0
>            Reporter: Robert Bradshaw
>            Assignee: Valentyn Tymofieiev
>            Priority: Major
>             Fix For: 2.7.0
>
>          Time Spent: 40m
>  Remaining Estimate: 0h
>
> When converting from Python 2 to Python 3, `a / b` becomes `a // b` only for ints, but
it is incorrect to do this substitution for floating point division. 
>  
> I noticed this change in the microbenchmarks, but we should do an audit to make sure
we haven't broken things elsewhere. 



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message