Rename the two different ChangePoint classes for clarity#151
Conversation
|
|
||
| @dataclass | ||
| class ChangePoint(_ChangePoint[TTestStats]): | ||
| class ChangePointHunter(ChangePointOtava[TTestStats]): |
There was a problem hiding this comment.
@Sowiks Did you have some plan what to do with these almost identical change point classes?
There was a problem hiding this comment.
I was planning to consolidate these two classes.
henrikingo
force-pushed
the
TwoChangePointClasses
branch
from
029b024 to
c937cbb
Compare
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.
|
@henrikingo CI is failing here. Can you take a look? |
|
Converting to draft. We should merge the two classes. This PR can eventually then be closed. Leaving it as draft for now. |
|
@henrikingo in #154 (comment) you mentioned that this PR should be merged first. Is there anything in this work stream I need to review now? Asking just to make sure I am not stalling your progress with reviews. In general, feel free to ping me if I am dropping the ball on your PRs that are ready. |
|
No and sorry if it wasn't clear. This PR currently renames two classes to emphasize that they are separate. Based on discussion (in this thread) what we should do is to merge the two classes into one. This is IMO not a hard dependency for #154 but would of course be nicer to do this first. |
|
Actual suggestion for refactor is now in #161 |
3 participants
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.