This is an archive of the discontinued LLVM Phabricator instance.

[LNT] Python 3 support: use Python 2's division behavior
ClosedPublic

Authored by thopre on Sep 20 2019, 1:40 AM.

Details

Summary

Adapt all divisions to use a call to old_div to maintain Python 2's
division behavior in legacy code. Also import Python 3's division
behavior by default. This was produced by running futurize's stage2
libfuturize.fixes.fix_division_safe.

Event Timeline

thopre created this revision.Sep 20 2019, 1:40 AM
examples/functions.py
33 ↗(On Diff #220980)

Clearly float division.

44 ↗(On Diff #220980)

Same.

lnt/external/stats/pstat.py
141

Clearly floor division (len(source) // len(addon)). Also for the rest of the changes in the file.

lnt/external/stats/stats.py
295

Except for division by zero (which is equally bad between floor division and float division), this is float division.

343

There are a few instances in the patch where the change is made where integer division is clearly stated to be the intent, and many cases where it is clear which division operation would have taken place in Python 2 (or cases where the floor division occurs only in degenerate cases and float division is fine anyway).

thopre updated this revision to Diff 222003.Sep 26 2019, 12:20 PM
thopre marked 125 inline comments as done.

Decide between float and integer division, getting rid of old_div

thopre added inline comments.
examples/functions.py
31 ↗(On Diff #220980)

math.pi is float as pointed out by Hubert and thus this is float division

42 ↗(On Diff #220980)

math.pi is float as pointed out by Hubert and thus this is float division

lnt/external/stats/pstat.py
141

len returns an integer as pointed out by Hubert so this is a floor division

146

len returns an integer as pointed out by Hubert so this is a floor division

153

len returns an integer as pointed out by Hubert so this is a floor division

158

len returns an integer as pointed out by Hubert so this is a floor division

lnt/external/stats/stats.py
295

As pointed by Hubert, either sum is 0 and thus both divisions give the same result, or sum is float by virtue of the 1.0/item and thus this should be float division.

343

len returns an int, therefore this is a floor division.

419

Second operand is a float due to being a number to the power 1.5, therefore this is a float division.

429

Denominator is a float due to being a number to the power 1.5, therefore this is a float division.

524

binsize is obtained by float division, therefore this is a float division.

700

math.sqrt returns a float so this is a float division.

710

mean refers to lmean in the same module which returns a float so this is a float division.

lnt/server/db/rules/rule_update_profile_stats.py
38

st_size being the size of f in bytes, it is an integer and thus this computes the floor.

lnt/server/reporting/analysis.py
83 ↗(On Diff #220980)

This is float division:

the only construction of ComparisonResult (the type of self) that can be the source of pct_delta uses in LNT seems to have samples and prev_samples (and thus ultimately self.delta and value) made of floats (samples or result of calling calc_geomean)
the only uses of pct_delta are by flask (e.g. lnt/server/ui/templates/v4_global_status.html) and max, both of which are happy with float values
pct_delta is set to 0.0 when the condition of the outermost "if" is false
lnt/server/reporting/dailyreport.py
49

day_start_offset is of type datetime.timedelta whose second attribute is an integer, therefore this is computing the floor.

lnt/server/reporting/summaryreport.py
50 ↗(On Diff #220980)

self.sum elements are initialized by 0. in _initialize and only ever changed by adding a value to it. Therfore this is float division.

68 ↗(On Diff #220980)

value ** 1.0 will yield a float, so this is clearly float division.

85 ↗(On Diff #220980)

While uses of NormalizedMean seems have values all come from calling lnt.util.stats.median and thus be integer, this makes more sense to be a float division because:

Mean._append uses the list elements in a float addition as explained above;
the same data that gets appended to NormalizedMean also gets appended to Mean in some cases

This suggests that the intent is to have float division.

505 ↗(On Diff #220980)

values is constructed by calling getvalue and all aggregation functions have their value of type float. This is therefore a float division

lnt/server/ui/util.py
15–19 ↗(On Diff #220980)

No use of safediv remains in LNT so this gets deleted in a separate patch.

73 ↗(On Diff #220980)

This method is only referenced 3 times, all in lnt/server/ui/views.py where in all cases it ends up being called with data which can be either integer or float AFAIK ("column" field of SampleField):

  • called once directly to fill the derived_stat attribute of the MatrixDataRequest which is not used anywhere
  • as an aggregate function when constructing a ComparisonResult that is being used in get_cell_value macro in lnt/server/ui/templates/v4_matrix.html by printing it with the %.4f specifier

It should therefore be consider float division, which makes sense for a mean.

315 ↗(On Diff #220980)

Like for ComparisonResult. all uses of pct_delta are fine with float and the initialization of pct_delta above suggest this is the intent.

lnt/server/ui/views.py
981 ↗(On Diff #220980)

values in samples comes from the column field of SampleField which it appears can be either integer or float. However this should be made a float division unconditionally IMHO. mean is used to mark the baseline line on the graph via the markings attribute of the grid attribute of the plot options passed to JQuery flot's plot function. This seems to be fine as a floating-point value and being a mean seems more sensible to be a floating-point value.

985 ↗(On Diff #220980)

color_offset is float so this is float division.

1153 ↗(On Diff #220980)

x iterate over elements of xs which is built from the first element in each tuple composing pts, which in turns is built by calling determine_x_value. Since that method return a float, this is a float division.

1898 ↗(On Diff #220980)

Both floor and float divisions behave the same so choosing to keep the existing code to minimize the diff.

thopre added inline comments.Sep 26 2019, 12:21 PM
lnt/external/stats/stats.py
852

As pointed out by the comment, r_den is a float (at least due to the call to math.sqrt) and thus this is a float division.

854

Denominator contains float operand and is thus float, this is a float division.

869

All variables involved are computed with float operations, this is thus a float division.

889

rs is computed with a float operand, it is therefore a float and this is a float division.

891

As mentioned in the comment, t is a float (bein computed from a sqrt call) and this is thus a float division.

922

xmean and ymean are float by virtue of being the result of a call to mean. This is a float division.

924–925

Denominator for the first line has some float operations, this is a float division and thus the line below too.

957–959

All divisions involve calls to sqrt or floating-point operands, these are float divisions.

981

r_num involves a float operand so this is a float division.

982

Float operands => float division.

984–985

float operand => first line uses a float division and thus the second line as well.

1011

Denominator is a sqrt return value, this is a float division

1042–1043

Denominator in first line is a sqrt return value, so it is a float division and therefore the second line as well.

1078–1079

denominator in first line is the return value of a sqrt call and so division on that line is a float division and thus the second line too

1172

sd is a sqrt return value, numerator has float operators => float division

1218

sqrt return value as denominator => float division

1251

se = sqrt return val => float division

1486

qap has float operands => float division

1490

em = float => float division

1493

Likewise.

1497–1499

ap, bp and app are constructed with the d values above which are float => float division

1523

coeff contains float numbers => float division

1546

Float operands in both numerator and denominator => float division

1581

Numerator and denominator formed from float division => float division

1750

As per comment, n is return value from len so this is integer division

1761

gap initialized to integer (see above) => integer division

2085
  • 1st outermost if: s is float from the reduce parameter => float division
  • 2nd outermost if: size is float => float division
  • outermost else:
    • 1st 2nd-level if: s initialized from float and is thus going to be a float at the end
    • 2nd-level else: s initialized from float with some entries replaced by float as well and potentially reshaped => float division

All cases are float division

2125

Denom always a float or float array (outermost else) => float division.

2167

shape contains integer and result of division used for indexing => integer division

2170

Likewise, not sure why the indx assignment is not hoisted out of the if but anyway, this is a float division

2234

Obviously float division

2352

sqrt => float division

2409

denom initialized with something to the power 1.5 => float division

2428

amoment returns a float => float division

2471–2472

Both lines involve float literals => float division

2474–2475

Involve numpy's sqrt => float division

2477

alpha and delta produced by float division => float division

2500

Uses result of numpy's sqrt => float division

2501–2502

numerator has a float literal => float division

2504–2505

Float literals => float division

2507

Numerator involves float division => float division

2508

innermost division: float literal => float division
outermost division: numpy's sqrt => float division

2746

mean returns a float so m is a float and thus this is a float division

2850

sqrt denominator => float division

2862

amean returns a float => float division

2889

mns is the result of amean which is of type float => float division

2978

denominator = numpy's sqrt => float division

3057

total is float => float division

3080–3084

withindf and betwdf are float => withinms and betweenms use float division => rho, t and prob use float division

3100

covar is the result of float division => float division

3118

r_den is the result of sqrt => float division

3120–3121

Division on 1st line involves float literals => float division
Division on 2nd line involves t which involves sqrt => float division

3139

Involves float literals => float division

3141

involves t which involves sqrt => float division

3170

numerator's operands are the result of amean, thus float => float division

3172–3173

1st line: float literals => float division
2nd line: t involves sqrt, thus float => float division

3205–3207

involves sqrt or float iterals => float division

3238–3239

1st line: r_den involves sqrt => float division
2nd line: float literals => float division

3241–3243

1st line: involves float literals => float division
2nd line: involves t which involves sqrt => float division
3rd line: involves float value => float division

3279

x & y are made of floats => r_num is float => float division

3281

float literals => float division

3283–3284

1st line: float literals => float division
2nd line: t involves sqrt and thus float => float division

3288

r_num is float as per above => float division

3315–3316

1st line: involves sqrt => float division
2nd line: involves result from 1st line => float division

3355

Involves numpy's sqrt => float division

3401

step is made of floats => float division

3436

df is float => float division

3439

denom is either 1 or float values from earlier float division and when it is 1 it is substituted by 1.0 below => float division gives same result

3474

f_exp is float => float division

3542

nominator involves float literals => float division

3588

sqrt => float division

3618–3619

se is result from sqrt => float division

3756

z cast to float => float division

3875

float literals => float division

3903

qap involves float literals => qap = float value => float division

3909

involves em which is float => float division

3912

Involves em which is float => float division

3916–3918

All variables involved defined by operations involving d which is the result from ealier float division => float division

3949

coeff is full of float literals => float division

3980

Involves float literals => float division

3984

float literals => float division

4027

Involves numpy's sqrt => float division

4055

Both variables are the result of float division => float division

4104

Both variables are the result of float divisions => float division

4263

n is the result of len => integer division

4274

gap is initialized by an integer division above => integer division

cmatthews accepted this revision.Nov 11 2019, 10:08 AM

Thanks for the comments. LGTM.

This revision is now accepted and ready to land.Nov 11 2019, 10:08 AM