Skip to content

Fix orig edivisive flow#141

Open
vishnuchalla wants to merge 1 commit intoapache:masterfrom
vishnuchalla:orig_edivisive
Open

Fix orig edivisive flow#141
vishnuchalla wants to merge 1 commit intoapache:masterfrom
vishnuchalla:orig_edivisive

Conversation

@vishnuchalla
Copy link
Copy Markdown

Description

Fix orig edivsive flow. I am running into below error if more than 10 datapoints are being used for analysis

vchalla@vchalla-thinkpadp1gen2:~/myforks/otava$ otava --config-file /tmp/otava-10.yaml analyze tiny10 --last 15 -P 0.001 --window 15 -M 0.0 --orig-edivisive=true
INFO: Computing change points for test tiny10...
Traceback (most recent call last):
  File "/home/vchalla/.local/bin/otava", line 7, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/vchalla/myforks/otava/otava/main.py", line 651, in main
    script_main()
  File "/home/vchalla/myforks/otava/otava/main.py", line 578, in script_main
    analyzed_series = otava.analyze(
                      ^^^^^^^^^^^^^^
  File "/home/vchalla/myforks/otava/otava/main.py", line 125, in analyze
    produced_report = report.produce_report(test.name, report_type)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/vchalla/myforks/otava/otava/report.py", line 52, in produce_report
    return self.__format_log_annotated(test_name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/vchalla/myforks/otava/otava/report.py", line 87, in __format_log_annotated
    change = [c for c in cp.changes if c.metric == col_name]
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/vchalla/myforks/otava/otava/report.py", line 87, in <listcomp>
    change = [c for c in cp.changes if c.metric == col_name]
                                       ^^^^^^^^
AttributeError: 'ChangePoint' object has no attribute 'metric'

Testing

Tested and verified in local

vchalla@vchalla-thinkpadp1gen2:~/myforks/otava$ otava --config-file /tmp/otava-15.yaml analyze tiny15 -P 0.001 --window 50 -M 0.0 --orig-edivisive
INFO: Computing change points for test tiny15...
time                       commit      metric1
-------------------------  --------  ---------
2026-01-01 00:00:00 +0000  c1              100
2026-01-02 00:00:00 +0000  c2              101
2026-01-03 00:00:00 +0000  c3               99
2026-01-04 00:00:00 +0000  c4              100
2026-01-05 00:00:00 +0000  c5              102
2026-01-06 00:00:00 +0000  c6              101
2026-01-07 00:00:00 +0000  c7              100
2026-01-08 00:00:00 +0000  c8               99
2026-01-09 00:00:00 +0000  c9              100
                                     ·········  
                                        +30.3%  
                                     ·········  
2026-01-10 00:00:00 +0000  c10             130
2026-01-11 00:00:00 +0000  c11             131
2026-01-12 00:00:00 +0000  c12             130
2026-01-13 00:00:00 +0000  c13             132
2026-01-14 00:00:00 +0000  c14             130

Signed-off-by: Vishnu Challa <vchalla@redhat.com>
@vishnuchalla
Copy link
Copy Markdown
Author

cc: @henrikingo @Gerrrr

@henrikingo
Copy link
Copy Markdown
Contributor

Unfortunately, rather than fixing the orig_edivisive flow, in master, we should actually remove it.

By orig_edivisive we mean the original, close to textbook implementation at MongoDB in 2017. Since we no longer want to depend on the external dependency, we cannot offer this option anymore.

Note that in the 0.7 branch you can still use --orig-edivisive, but as far as I can tell, this bug isn't happening in that branch.

@Gerrrr
Copy link
Copy Markdown
Contributor

Gerrrr commented Mar 29, 2026

Unfortunately, rather than fixing the orig_edivisive flow, in master, we should actually remove it.

+1.

Note that in the 0.7 branch you can still use --orig-edivisive, but as far as I can tell, this bug isn't happening in that branch.

@vishnuchalla if you find issues with --orig-edivisive in 0.7.0, we can fix it there and do a bugfix release.

@henrikingo
Copy link
Copy Markdown
Contributor

Closing. If we misunderstood something, please just reply here and reopen.

@henrikingo henrikingo closed this Mar 30, 2026
@henrikingo
Copy link
Copy Markdown
Contributor

Oh dear... My sincerest apologies. I was looking into something unrelated yesterday and realized that there is a completely valid --orig-edivisive flow also in the new algorithm in main branch / 0.8.0 and higher. I had forgotten that @Sowiks did re-implement a version of the algorithm that is entered at

def compute_change_points_orig(series: Sequence[SupportsFloat], max_pvalue: float = 0.001, seed: Optional[int] = None) -> Tuple[PermCPList, Optional[PermCPList]]:

...and your patch is against that.

I will re-open this and look at your patch later.

@henrikingo henrikingo reopened this Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants