add Python 3 #28

brennv · 2017-08-28T21:29:23Z

No description provided.

brennv · 2017-08-31T09:10:47Z

@bdewilde could you review these commits?

bdewilde · 2017-08-31T14:27:11Z

src/luminol/algorithms/anomaly_detector_algorithms/default_detector.py

@@ -9,8 +9,8 @@
 distributed under the License is distributed on an "AS IS" BASIS,
 WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 """
-from exp_avg_detector import ExpAvgDetector
-from derivative_detector import DerivativeDetector
+from luminol.algorithms.anomaly_detector_algorithms.exp_avg_detector import ExpAvgDetector


Imports in Py2 vs Py3 have been an occasionally confusing issue... If we're only supporting Py2.7, I don't think we need the from __future__ import absolute_import directive, but may be worth double-checking. Also, I've been told that relative imports, e.g. from .exp_avg_detector import ExpAvgDetector, are preferable in subpackage modules but not in __init__.py modules, but that may be a matter of preference.

This is a good point. Explicit relative imports for subpackage modules seems to be the convention.

After trying it out, with the levels we're traversing it would look like:

from ... import utils from ...algorithms.anomaly_detector_algorithms.exp_avg_detector import ExpAvgDetector from ...algorithms.anomaly_detector_algorithms.derivative_detector import DerivativeDetector from ...algorithms.anomaly_detector_algorithms import AnomalyDetectorAlgorithm from ...constants import * from ...modules.time_series import TimeSeries

I think in this case I'm partial to the absolute imports, but either is fine with me.

Yeah, I'm mostly sure it doesn't matter. If this works in both Py2 and Py3, there's probably no real reason to change it.

Ok cool, I'll leave it as is for now.

Thank you for the review :)

bdewilde · 2017-08-31T14:30:53Z

src/luminol/algorithms/correlator_algorithms/cross_correlator.py

@@ -87,7 +88,7 @@ def _find_allowed_shift(self, timestamps):
        param list timestamps: timestamps of a time series.
        """
        init_ts = timestamps[0]
-        residual_timestamps = map(lambda ts: ts - init_ts, timestamps)
+        residual_timestamps = list(map(lambda ts: ts - init_ts, timestamps))


A list comprehension would probably be more readable: [ts - init_ts for ts in timestamps].

bdewilde · 2017-08-31T14:33:45Z

src/luminol/algorithms/correlator_algorithms/cross_correlator.py

@@ -9,6 +9,7 @@
 distributed under the License is distributed on an "AS IS" BASIS,
 WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 """
+from builtins import map


Looks like the only use of map is Line 91; if you use a list comprehension, or if you wrap the map in a list() call, is there any need for this import?

If we transition the list/lambda/map statements to list comprehensions then we shouldn't need this.

bdewilde · 2017-08-31T14:43:20Z

src/luminol/modules/time_series.py

@@ -43,7 +45,7 @@ def timestamps_ms(self):
        """
        Return list of timestamp values in order by milliseconds since epoch.
        """
-        return map(lambda ts: ts * 1000, self.timestamps)
+        return list(map(lambda ts: ts * 1000, self.timestamps))


As before, list comprehension may be more readable: [ts * 1000 for ts in self.timestamps]

bdewilde · 2017-08-31T14:44:11Z

src/luminol/modules/time_series.py

@@ -278,7 +280,7 @@ def add_offset(self, offset):
        :param int offset: The number of seconds to offset the time series.
        :return: `None`
        """
-        self.timestamps = map(lambda ts: ts + offset, self.timestamps)
+        self.timestamps = list(map(lambda ts: ts + offset, self.timestamps))


list comprehension?

bdewilde · 2017-08-31T14:44:21Z

src/luminol/modules/time_series.py

@@ -288,7 +290,7 @@ def normalize(self):
        """
        maximum = self.max()
        if maximum:
-            self.values = map(lambda value: value / maximum, self.values)
+            self.values = list(map(lambda value: value / maximum, self.values))


list comprehension?

... I know this is a matter of preference, but I have a strong preference for list comprehensions over map in cases like this.

I also prefer using list comprehensions over lambda/map.

bdewilde · 2017-08-31T14:46:08Z

src/luminol/tests/test_anomaly_detector.py

-        anom_scores[timestamp] = -1 * diff_percent
-
-    self.anom_scores = TimeSeries(self._denoise_scores(anom_scores))
+    Copy of DiffPercentThreshold Algorithm from algorithms/anomaly_detector_algorithms/diff_percent_threshold.py to test


Not sure what all this is about...

This just fixes the spacing convention from 2 to 4 spaces. With #27 recently merged, it's already committed.

bdewilde · 2017-08-31T14:48:52Z

src/luminol/utils.py

@@ -42,7 +43,8 @@ def read_csv(csv_name):
    :return dict: a dictionary represents the data in file.
    """
    data = {}
-    if not isinstance(csv_name, (str, unicode)):
+    # if not isinstance(csv_name, (str, unicode)):


In this case, if you don't care whether csv_name is unicode or bytes, you could do something like this:

import sys if int(sys.version[0]) == 2: str_types = (str, unicode) else: str_types = (bytes, str)

Then if not isinstance(csv_name, str_types): ...

I like this, this is a cleaner approach.

brennv · 2017-08-31T21:31:25Z

@RiteshMaheshwari i think we're ready to merge :)

bdewilde · 2017-09-07T19:03:24Z

Just a friendly nudge to @RiteshMaheshwari or whomever could approve and merge in these changes... 👋

RiteshMaheshwari · 2017-09-07T19:22:22Z

I am reaching out to folks who know this codebase better than me to give a review.

RiteshMaheshwari · 2017-09-07T21:19:23Z

src/luminol/algorithms/anomaly_detector_algorithms/exp_avg_detector.py

@@ -34,6 +34,8 @@ def __init__(self, time_series, baseline_time_series=None, smoothing_factor=None
        """
        super(ExpAvgDetector, self).__init__(self.__class__.__name__, time_series, baseline_time_series)
        self.use_lag_window = use_lag_window
+        if smoothing_factor is None:
+            smoothing_factor = 0


Wouldn't it be better to change the method definition to set default smoothing_factor to 0 instead?

Will do. This is would definitely be a cleaner way to do it.

RiteshMaheshwari · 2017-09-07T21:54:17Z

src/luminol/correlator.py

@@ -113,4 +116,6 @@ def is_correlated(self, threshold=None):
        Compare with a threshold to determine whether two timeseries correlate to each other.
        :return: a CorrelationResult object if two time series correlate otherwise false.
        """
+        if threshold is None:
+            threshold = 0


Same comment as above, why not just change the method definition?

Yes, this is definitely better. Making the change now.

brennv · 2017-09-13T01:19:47Z

@RiteshMaheshwari Changes made, please let me know if there's anything else.

RiteshMaheshwari · 2017-09-13T18:05:28Z

Got it reviewed internally as well. LGTM.

brennv and others added 12 commits August 28, 2017 14:28

add 3.6

2b333c2

fix import

db2b655

fix next, map

cab3c08

fix unicode

c633a26

add future

1cca2d8

fix dict_items

5b01a80

fix map

46ea963

fix None comparison

31cdd49

remove print

fd480ad

fix assertEquals

af4de3c

fix division

2c29c51

fix dict_values

66b1a50

brennv force-pushed the py-three branch from 50f2762 to 66b1a50 Compare August 28, 2017 21:55

This was referenced Aug 28, 2017

Python3 roadmap #22

Closed

clean up #27

Merged

AttributeError: 'module' object has no attribute 'anomaly_detector' #17

Open

bdewilde reviewed Aug 31, 2017

View reviewed changes

brennv added 3 commits August 31, 2017 13:24

Merge remote-tracking branch 'linkedin/master' into py-three

053e9d8

use list comp

a8eeb4d

add str_types

866689b

RiteshMaheshwari requested changes Sep 7, 2017

View reviewed changes

fix NoneType comparison

5099dda

RiteshMaheshwari approved these changes Sep 13, 2017

View reviewed changes

RiteshMaheshwari merged commit 096da9d into linkedin:master Sep 13, 2017

brennv mentioned this pull request Sep 28, 2017

Issue when importing modules #6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Python 3 #28

add Python 3 #28

brennv commented Aug 28, 2017

brennv commented Aug 31, 2017

bdewilde Aug 31, 2017

brennv Aug 31, 2017

brennv Aug 31, 2017

bdewilde Aug 31, 2017

brennv Aug 31, 2017

bdewilde Aug 31, 2017

brennv Aug 31, 2017

bdewilde Aug 31, 2017

brennv Aug 31, 2017

bdewilde Aug 31, 2017

brennv Aug 31, 2017

bdewilde Aug 31, 2017

brennv Aug 31, 2017

bdewilde Aug 31, 2017

brennv Aug 31, 2017

bdewilde Aug 31, 2017

brennv Aug 31, 2017

bdewilde Aug 31, 2017

brennv Aug 31, 2017

brennv commented Aug 31, 2017 •

edited

Loading

bdewilde commented Sep 7, 2017

RiteshMaheshwari commented Sep 7, 2017

RiteshMaheshwari Sep 7, 2017

brennv Sep 8, 2017

RiteshMaheshwari Sep 7, 2017

brennv Sep 8, 2017

brennv commented Sep 13, 2017

RiteshMaheshwari commented Sep 13, 2017

add Python 3 #28

add Python 3 #28

Conversation

brennv commented Aug 28, 2017

brennv commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brennv commented Aug 31, 2017 • edited Loading

bdewilde commented Sep 7, 2017

RiteshMaheshwari commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brennv commented Sep 13, 2017

RiteshMaheshwari commented Sep 13, 2017

brennv commented Aug 31, 2017 •

edited

Loading