Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Serialisation of (online) state for online detectors #604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Serialisation of (online) state for online detectors #604
Changes from 37 commits
2171395
68883d4
944a65d
672fbcc
9411d54
d3c397a
06a032b
9ea9339
faa483b
ec48bb6
6deb17f
272ef3a
e9c3685
f73fbbf
09c4305
1774ee9
d9d2e25
975094e
363b435
cfc8918
30bc57a
1d81d35
2d9c5cf
bb53453
bb4035a
430ebd5
91e1b61
9e84410
5dea156
785365d
7ce6476
a2bce2b
b2806cc
2d4b6a3
67915b8
feb9fda
aa6b0d3
15d1dfa
da56714
6240fdf
5daf1b1
c7d72c7
d190589
4de6fad
7b824d8
d888276
1026713
828e486
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should parameters have type hints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 15d1dfa. Note: I also updated the pre-existing
get_config
andset_config
methods here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a private attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #604 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary when loading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so that
self.state_dir
is set (and converted fromstr
topathlib.Path) when
load_stateis called as well as when
save_state` is called.I thought it might be helpful to have
state_dir
as a public attribute so that a user could see interrogate the detector to see where state was loaded from. Although thinking about it more, for the backend detectors one would have to dodetector._detector.state_dir
(access a private attribute) anyway. I guess we'd probably want to define a@property
if we actually want to support this functionality properly...Happy to just make it private if you think its better though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a lot of duplicated code that's exactly the same as for
BaseMultiDriftOnline
which suggests we may want to refactor using functions instead of methods or a mixin class? Or perhaps the class hierarchy needs to be updated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this seems to apply to other methods too, so perhaps is a more widespread problem requiring a refactoring later...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. Having a
BaseDriftOnline
class for generic methods, or a mix-in both seem much nicer than this current pattern. I'll have a rethink 👍🏻There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reduce duplication, 5daf1b1 adds a
BaseDriftOnline
class. @jklaise @mauicv could I get your thoughts on the design ofBaseDriftOnline
please? I've gone with a parent class rather than mix-in since it seems strange to define a mix-in inalibi_detect/base.py
when it is only to be used in two classes (BaseMultiDriftOnline
andBaseUniDriftOnline
). I also decided to put it inalibi_detect/cd/base_online.py
rather thanalibi_detect/base.py
since at the moment the concept of "online" detectors is specific to drift (this may change if we decide stateful outlier detectors are in fact "online").There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM however noting that there's quite a few abstract methods, some of which (not all?) are implemented in the
Multi/Uni
abstract child classes, which come with their own set of abstract methods... Worried that this may become a bit tricky to keep track of. As a minimum, would group all abstract methods to come after each other and add docstrings on expected implementation and also, where valid, which of theMulti/Uni
classes implement these methods (+ type hints as always).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #604 (comment) wrt to type hints, not sure on best approach here.
Wrt to the abstract methods, if they are missing from the
Multi/Uni
child classes that will be because they are instead defined in the next subclass down i.e.LSDDDriftOnlineTorch._initialise_state
orCVMDriftOnline._configure_thresholds
...We could move the abstract methods such as
_configure_thresholds
back to their respectiveMulti/Uni
abstract class, at the cost of more duplication (but maybe less complexity?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the new base class, and moved state methods to
StateMixin
. See #604 (comment).