luminaire icon indicating copy to clipboard operation
luminaire copied to clipboard

diff_order seems to be hard coded to be diff order of 2

Open pdurham2 opened this issue 2 years ago • 2 comments

When diff_order is applied in lad_filtering.py, the value passed to np.diff is fixed as 2. Is this intended or should diff_order be passed instead?

if diff_order:
  actual_previous_per_diff = [interpolated_actual_previous[-1]] \
      if diff_order == 1 else [interpolated_actual_previous[-1], np.diff(interpolated_actual_previous)[0]]
  seq_tail = interpolated_actual_previous + [interpolated_actual]
  interpolated_actual = np.diff(seq_tail, 2)[-1]

pdurham2 avatar Oct 20 '22 18:10 pdurham2

@pdurham2 I think your observation is correct. Based on the logic for this part of the code, this should be interpolated_actual = np.diff(seq_tail, diff_order)[-1].

Please feel free to make the change and add me as a reviewer. We should also test the change by training and scoring the filtering model using an example dataset (you can use any dataset from https://github.com/zillow/luminaire/tree/master/luminaire/tests/datasets) to check if everything is working as expected.

sayanchk avatar Oct 22 '22 06:10 sayanchk

@sayanchk sounds good, will do that soon - thanks for confirming!

pdurham2 avatar Oct 22 '22 14:10 pdurham2