fixed SR begining/ending spikes #396

RobertSamoilescu · 2021-11-30T11:29:44Z

This PR addresses the spikes at the beginning and ending of the scores reported in #290.
Previous implementation was performing the convolution over the entire spectrum returned by the FFT, thus including the initial bias term. Besides the asymmetry introduced by the convolution operation, which resulted in complex numbers when applying the IFFT, the convolution operation was introducing a considerable bias since the numpy implementation pads the signal with 0 before convolving. The same bias is also valid in time domain. To address the bias, I introduced an option to chose the padding strategy:

constant - pads the signal with 0
replicate - replicates the most extreme value
reflect - reflects the sginal

After benchmarking the various padding strategy, I concluded that the reflect strategy works best, with the window_amp centered in the current value (i.e., side=bilateral) .

All the issues listed above were significant when the synthetic signal from the example notebook was lifted (i.e., add an offset of 100).

In addition to the previous fixes, I modified the implementation to performe the averaging in time domain over the previous window_local data points (i.e., is a local average of the preceding window_local points for the current index) as suggested in the paper.

review-notebook-app · 2021-11-30T11:29:47Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

jklaise · 2021-11-30T11:45:42Z

alibi_detect/od/sr.py

+# padding values
+PADDING_CONSTANT = 'constant'
+PADDING_REPLICATE = 'replicate'
+PADDING_REFLECT = 'reflect'
+PADDINGS = [PADDING_CONSTANT, PADDING_REFLECT, PADDING_REPLICATE]
+
+# padding sides
+SIDE_BILATERAL = 'bilateral'
+SIDE_LEFT = 'left'
+SIDE_RIGHT = 'right'
+SIDES = [SIDE_BILATERAL, SIDE_RIGHT, SIDE_LEFT]


Might be slightly nicer to make these enums, inheriting from both str and enum.Enum, e.g. class Padding(str, Enum) so then can refer to e.g. Padding.CONSTANT and you could validate user passed strings against allowed values using one of the methods here: https://stackoverflow.com/questions/29503339/how-to-get-all-values-from-python-enum-class/29503414

ascillitoe · 2021-11-30T11:45:59Z

I haven't looked through in detail yet, but two minor thoughts; given the relative complexity of the new kwargs, it might be worth updating doc/source/od/methods/sr.ipynb with more details, and also adding some basic tests for the different padding strategies and padding sides?

jklaise · 2021-11-30T11:48:18Z

alibi_detect/od/sr.py

+                 padding_amp_method: str = PADDING_REFLECT,
+                 padding_local_method: str = PADDING_REFLECT,
+                 padding_amp_side: str = SIDE_BILATERAL,


Since this is user facing, I think we should be more explicit, e.g. types as Literal['constant', 'replicate', 'reflect'] = 'reflect' which unfortunately introduces some name duplication (unless there's a way to define Literal types given a list of string constant from the padding enum?) but is likely worth it for clarity.

jklaise · 2021-11-30T11:49:10Z

alibi_detect/od/sr.py

+                 method: str = "replicate",
+                 side: str = 'bilateral'


more explicit typing via Literal as above

Also, consistent use of ' over " please.

jklaise · 2021-11-30T14:44:58Z

alibi_detect/od/sr.py

 import logging
 import numpy as np
-from typing import Dict
+from typing import Dict, Literal


Small thinkg I forgot, Literal is not in the standard lib for Python 3.6 so for support would need to do something like this:
https://github.com/SeldonIO/alibi/blob/0ccf1b726b6a865b5a94d727847653ffc7e68a4f/alibi/explainers/ale.py#L11-L14

We will drop Python 3.6 support very soon though, though probably not for the next release so best to put this in. @ascillitoe

Actually it is only present in standard lib for Python 3.8+ so we won't be able to get rid of the version check for a while as 3.7 will be supported for some time.

ascillitoe · 2021-11-30T15:12:11Z

alibi_detect/od/sr.py

 import logging
 import numpy as np
 from typing import Dict
 from alibi_detect.base import BaseDetector, ThresholdMixin, outlier_prediction_dict

+if sys.version_info >= (3, 8):


I'm going to be adding Literal in lots of places in a future PR, so it might be useful to move this to alibi_detect._types or alibi_detect.utils._types. Although I can refactor this in the future if its easier...

alibi_detect/od/tests/test_sr.py

ascillitoe · 2021-11-30T17:30:36Z

@RobertSamoilescu I've opened this #398 to remind us to uncomment data.meta['attack_type'] == attack once the dataset is fixed.

jklaise · 2021-12-01T10:46:20Z

alibi_detect/od/sr.py

+        assert X.shape[0] > self.conv_amp.shape[0], "The length of the input signal should be greater " \
+                                                    "than the amplitude window"


This sounds like we should raise an error instead if this is triggered by user error? Or is this something that should never really happen?

I will raise an error

ascillitoe

Mostly looks good to me now, although still think it's worth updating doc/source/od/methods/sr.ipynb to include the new kwarg's.

jklaise

LGTM. I'll defer to @arnaudvl to have a look at the maths.

alibi_detect/od/sr.py

RobertSamoilescu requested review from jklaise and arnaudvl November 30, 2021 11:29

jklaise reviewed Nov 30, 2021

View reviewed changes

RobertSamoilescu changed the title ~~fixed begining/ending spikes~~ fixed SR begining/ending spikes Nov 30, 2021

jklaise reviewed Nov 30, 2021

View reviewed changes

ascillitoe reviewed Nov 30, 2021

View reviewed changes

alibi_detect/od/tests/test_sr.py Outdated Show resolved Hide resolved

ascillitoe mentioned this pull request Nov 30, 2021

Fix metadata in attack dataset #398

Open

RobertSamoilescu force-pushed the sr_spikes branch from 8248f86 to e71c36c Compare November 30, 2021 18:50

jklaise reviewed Dec 1, 2021

View reviewed changes

ascillitoe suggested changes Dec 1, 2021

View reviewed changes

This was referenced Dec 1, 2021

SpectralResidual results in an odd plot with instance_scores spiking only at the beginning and at the end and flattening at everywhere intermediate. #290

Closed

Can't run Spectral Residual #382

Closed

RobertSamoilescu force-pushed the sr_spikes branch 2 times, most recently from 8768da6 to cc36d78 Compare December 1, 2021 12:59

jklaise approved these changes Dec 6, 2021

View reviewed changes

arnaudvl reviewed Dec 21, 2021

View reviewed changes

alibi_detect/od/sr.py Show resolved Hide resolved

arnaudvl approved these changes Dec 21, 2021

View reviewed changes

RobertSamoilescu added 7 commits January 6, 2022 07:27

resolved conflicts

65fbd57

moved constants into enumerate. included types

6ced013

included padding tests

32a31ae

fixed Literal typing

daf59e7

included _types

814f0f0

commented data.meta['attack_type'] == attack

089aa09

refactored the test

4cbe484

RobertSamoilescu added 4 commits January 6, 2022 07:31

updated docs and error handling

c298c24

included new kwargs

44f555d

add example

198c668

fixed minor docs warning

972191e

RobertSamoilescu force-pushed the sr_spikes branch from cc36d78 to 972191e Compare January 6, 2022 14:03

jklaise approved these changes Jan 6, 2022

View reviewed changes

add new kwargs in doc/source/od/methods/sr.ipynb

fa9bb21

ascillitoe approved these changes Jan 6, 2022

View reviewed changes

jklaise merged commit e29cd7b into SeldonIO:master Jan 6, 2022

This was referenced Mar 10, 2022

Unexplainable error with SpectralResidual: RuntimeWarning: invalid value encountered in subtract res_amp = log_amp - ma_log_amp #291

Closed

last value is always anomaly in SpectralResidual #153

Closed

Improve extrapolation SR outlier detector #71

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed SR begining/ending spikes #396

fixed SR begining/ending spikes #396

RobertSamoilescu commented Nov 30, 2021

review-notebook-app bot commented Nov 30, 2021

jklaise Nov 30, 2021

ascillitoe commented Nov 30, 2021

jklaise Nov 30, 2021

jklaise Nov 30, 2021

jklaise Dec 1, 2021

jklaise Nov 30, 2021

jklaise Nov 30, 2021

ascillitoe Nov 30, 2021

ascillitoe commented Nov 30, 2021

jklaise Dec 1, 2021

RobertSamoilescu Dec 1, 2021

ascillitoe left a comment

jklaise left a comment

		assert X.shape[0] > self.conv_amp.shape[0], "The length of the input signal should be greater " \
		"than the amplitude window"

fixed SR begining/ending spikes #396

fixed SR begining/ending spikes #396

Conversation

RobertSamoilescu commented Nov 30, 2021

review-notebook-app bot commented Nov 30, 2021

Choose a reason for hiding this comment

ascillitoe commented Nov 30, 2021

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

ascillitoe commented Nov 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascillitoe left a comment

Choose a reason for hiding this comment

jklaise left a comment

Choose a reason for hiding this comment