Repository navigation
Fix orig edivisive flow - #141
Conversation
Signed-off-by: Vishnu Challa <vchalla@redhat.com>
|
cc: @henrikingo @Gerrrr |
|
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. |
+1.
@vishnuchalla if you find issues with --orig-edivisive in 0.7.0, we can fix it there and do a bugfix release. |
|
Closing. If we misunderstood something, please just reply here and reopen. |
|
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 Line 280 in 1ceb153 ...and your patch is against that. I will re-open this and look at your patch later. |
|
Ok yes,I caught up with this. Working indepently on a different patch, I end up needing exactly this patch for --orig-edivisive +1 |
|
I think your patch looks good except for this one linting failure. If you move the TTestSignificanceTester one line up, I'll hit the button to rerun tests for you. In the mean time, do you already have a signed contributor agreement on file with the ASF? If not:
|
Signed-off-by: Vishnu Challa <vchalla@redhat.com>
8f45408 to
76ed75d
Compare
@henrikingo Singed and emailed the ICLA. Also fixed the linting issue. PTAL once you get sometime. Thank you. |
The recent work in #96 to replace the original external dependency on the so called "signal processing" repository with our own implementation, introduced new classess ChangePoint and CandidateChangePoint, in change_point_divisive/base.py but also left in place the original ChangePoint class in analysis.py. These come together in series.py, where the newer is renamed as _ChangePoint() and also acts as a parent to older class, thus aligning their signature as much as possible. It turns out having two similarly named classes can be a source of confusion and bugs. For example, in #141 vishnuchalla fixes a bug that is due to this and has essentially blocked the --orig-edivisive code path completely. This patch is an effort to make the existence of two separate classes very explicit, by renaming them to ChagePointHunter and ChangePointOtava based on their "lineage". A test case is added to exercise the --orig-edivisive code path. The test fails, as predicted by #141. The test is now cmmented out. The bug is due to a missing cp.metric property in one variation of the ChangePoint class. Note that this patch is intended more for discussion than to merge.
|
Whether testing the full output of |
I think the tests just need a re-run? Updated with this patch. |
|
The merge re-introduced 413d774 |
715d9a5 to
46e0ba7
Compare
Oops, fixed. |
|
Thanks. And nice to have you here @vishnuchalla ! |
The recent work in #96 to replace the original external dependency on the so called "signal processing" repository with our own implementation, introduced new classess ChangePoint and CandidateChangePoint, in change_point_divisive/base.py but also left in place the original ChangePoint class in analysis.py. These come together in series.py, where the newer is renamed as _ChangePoint() and also acts as a parent to older class, thus aligning their signature as much as possible. It turns out having two similarly named classes can be a source of confusion and bugs. For example, in #141 vishnuchalla fixes a bug that is due to this and has essentially blocked the --orig-edivisive code path completely. This patch is an effort to make the existence of two separate classes very explicit, by renaming them to ChagePointHunter and ChangePointOtava based on their "lineage". A test case is added to exercise the --orig-edivisive code path. The test fails, as predicted by #141. The test is now cmmented out. The bug is due to a missing cp.metric property in one variation of the ChangePoint class. Note that this patch is intended more for discussion than to merge.
* Rename the two different ChangePoint classes for clarity The recent work in #96 to replace the original external dependency on the so called "signal processing" repository with our own implementation, introduced new classess ChangePoint and CandidateChangePoint, in change_point_divisive/base.py but also left in place the original ChangePoint class in analysis.py. These come together in series.py, where the newer is renamed as _ChangePoint() and also acts as a parent to older class, thus aligning their signature as much as possible. It turns out having two similarly named classes can be a source of confusion and bugs. For example, in #141 vishnuchalla fixes a bug that is due to this and has essentially blocked the --orig-edivisive code path completely. This patch is an effort to make the existence of two separate classes very explicit, by renaming them to ChagePointHunter and ChangePointOtava based on their "lineage". A test case is added to exercise the --orig-edivisive code path. The test fails, as predicted by #141. The test is now cmmented out. The bug is due to a missing cp.metric property in one variation of the ChangePoint class. Note that this patch is intended more for discussion than to merge. * Unify the two ChangePoint classes and add container classes * Unify the ChangePoint_ class in hunter code and the new ChangePoint introduced by the new edivisive implementation Then it got out of hand a bit ... * Separate index and timestamp into different domains. cp.index is used in the context of a single metric and its history of results. Time and commit otoh are on the ChangePointGroup level (essentially a "row"). Note that different metrics can now have different cp.index for the same cpg.time or cpg.attributes['commit'], if they have a different history. * Introduce a ChangePoints class which is just a list of ChangePointGroups but actually comes with 2 different implementations. The last one is supposed to become the class you are left holding once all the change points are computed. Until now we had lots of nice classes for each step of computation, but in the end you were left holding a dict[str, ChangePointGroup]. The new class now encapsulates that dict, * Refactor common code between the significance testers This moves stats and functionality up towards parent classes so that generic stats like mean are always computed for all variants. In fact TTestStat is now an empty class, it's functionality fully absorbed by the parent. (But note that the class name/type itself carries information about the pvalue. * ruff/flake/idk * format * Re-enable tox alsso in gh actions * fix: timezone in 3.10 compatible way * bw compat: must use timezone.UTC * Simplify __compute_change_points() by using ChangePoints earlier * Simplify ChangePoints() constructor Create separate copy(), from_dict(), from_list() constructors Constructors now just create an empty container Add tests for unified ChangePoint classes; fix many bugs Also move BaseStats compute logic into a calculate() staticmethod so copy() is a plain replace(). * Make ChangePoints._change_points private by adding _ * ChangePointsByMetric()[n] is not well defined, go via by_time() Other small fixes * Visit all change points and columns, not just values()[0] * Add comment about why indexes will not be aligned or can have gaps * TTestStats: Add tstatistic and degrees_of_freedom Add a few t-test specific parameters to the TTestStats class * Add to_json() serializers to subclasses of BaseStats Note: No from_json(), as the higher level from_json() functions can easily just call the constructor directly. * Self review, various fixes Added typing annotations and docstrings where I felt they were lacking. Cleaned up the serialization and insert to bigquery and postgres. Both of which continue to insert ChangePoint(s), even if they take a ChangePointGroup as arguments. This is because .time and .attributes are now both a property of the ChangePointGroup. * Add pytest-cov for code coverage * Add missing unit test coverage In fact, went all the way and achieved 100% coverage in the files mainly touched by this PR. Lint and format edits too. * review: Remove __eq__() for ChangePoint class * review: PermutationStats.copy() wasn't copying NDArray deeply * review: Fix and add test for ReportType.REGRESSIONS_ONLY * review: datatype should be datetime * review: to_json(): fix the case with multiple metrics * review: Tests cleanup and fixes * lint & format (tox) * review: Add comment blocks to ChangePointsBy* classes * fix: handle empty ChangePointsByMetric length * fix: refresh change points by time after append * fix: update weak change points after append * fix: serialize weak change points from weak cache * fix: restore analyzed series from grouped json --------- Co-authored-by: Alex Sorokoumov <aleksandr.sorokoumov@gmail.com>
3 participants
Description
Fix orig edivsive flow. I am running into below error if more than 10 datapoints are being used for analysis
Testing
Tested and verified in local