abrignoni / abrignoni/ALEAPP

convert_unix_ts_to_utc: float() coercion differs between the five cores

オープン
#1,161 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
Python
スター
908
フォーク
244
平均マージ
10時間 18分
マージ済み PR(30日)
288

説明

`convert_unix_ts_to_utc` in `scripts/ilapfuncs.py` differs between the five cores. ALEAPP calls the sizing helper directly. iLEAPP, RLEAPP, VLEAPP and DLEAPP first coerce the input with `float(ts)` and return the input unchanged if that raises.

ALEAPP:

```python
def convert_unix_ts_to_utc(ts):
if ts:
ts = convert_unix_ts_in_seconds(ts)
return _UNIX_EPOCH_UTC + timedelta(seconds=ts)
else:
return ts
```

The other four have this ahead of the `convert_unix_ts_in_seconds` call:

```python
try:
ts = float(ts)
except (ValueError, TypeError, OSError, OverflowError):
return ts
```

The rest of the function is identical in all five after the timestamp fix (#1158 here, and the matching iLEAPP #1989, RLEAPP #423, VLEAPP #135, DLEAPP #79). This is the only remaining difference, and it is worth settling before the cores are consolidated.

## What the coercion changes

Three input classes behave differently. None of them is currently produced by any artifact that was measured, see below.

1. A decimal string such as `'1755000000.5'`. The int path raises `ValueError`; the float path decodes it.
2. A non-numeric string. The int path raises `ValueError`; the float path catches it and returns the string unchanged. For a column declared `datetime` that means a non-timestamp value reaches the report instead of an error.
3. A 16 digit value at the top of the microsecond range. `float()` is not exact above 2**53, so `9999999999999999` rounds up to `10000000000000000`, which crosses the nanosecond threshold in the sizing helper:

```
int path -> 9999999999 -> 2286-11-20 17:46:39+00:00
float path -> 10000000 -> 1970-04-26 17:46:40+00:00
```

Point 3 is a defect the four coercing cores have and ALEAPP does not, so it argues for removing the coercion rather than adding it. Note this value is at a range boundary and was constructed, not observed.

## What was measured

Instrumented runs recording the raw input type of every value reaching `convert_unix_ts_to_utc`, and whether adding the coercion changes the decoded result.

ALEAPP, all 18 registered corpora, complete:

- 136,145 values
- types seen: `float` 77,314, `int` 58,831. No strings.
- values whose decoded result the coercion would change: 0

iLEAPP, 14 of 24 registered corpora, partial (the sweep was stopped):

- 831,902 values
- types seen: `float` 731,665, `int` 100,213, `str` 24
- values where the int path would raise, so the coercion is load bearing: 0
- values whose decoded result the coercion would change: 0

So on everything measured, adding the coercion to ALEAPP is a no-op, and removing it from iLEAPP would also be a no-op. The 24 string inputs in iLEAPP all parse as integers, but they show that string inputs do occur there, so removal is not free by inspection.

RLEAPP, VLEAPP and DLEAPP have not been measured at all.

## What would settle it

Finish the iLEAPP sweep and run the same measurement on RLEAPP, VLEAPP and DLEAPP. If no core has an input where the coercion is load bearing, removing it from the four and leaving ALEAPP as it is makes all five identical, drops the 16 digit boundary defect, and keeps a bad input loud rather than passing it through to the report.

If some core does depend on it, the alternative is to add it to ALEAPP and fix the boundary separately, for example by comparing magnitude before coercing.

No change proposed here. Filed so the decision is not lost.

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。