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
Support timed belief db mixin #53
Conversation
By indexing timed beliefs by their event start and their sensor id, queries can become much faster.
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.
Good stuff!
I have only smallish comments, only two I really think need to be done.
@@ -136,3 +136,6 @@ def test_custom_source_and_beliefs_with_mixin(db): | |||
|
|||
assert "my_belief_source" in db.tables.keys() | |||
assert "my_timed_belief" in db.tables.keys() | |||
|
|||
bdf = JoyfulBeliefInCustomTable.query(session, sensor) |
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.
Is this supposed to be query_all
? If so, why does this assertion work? Is bdf
maybe an sqlachemy.Query
object instead of a BeliefsDataFrame
?
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.
I used query
on purpose, to have a test that triggers the corresponding FutureWarning. I've been following this strategy for all deprecations, for two reasons:
- The test suite will show FutureWarnings for upcoming deprecations, which keeps reminding developers to actually deprecate at some point.
- The developer that implements the actual deprecation will break the test, and is therefore made aware of the fact that dependent code may break (and hopefully, reflect on whether enough time has passed since the deprecation was announced).
I'm glad you noticed this, because giving it some more thought I can come up with a few things to improve this strategy:
- Add a todo to the assert statement to amend the upcoming deprecation.
- Note the version at which the deprecation was announced.
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.
Almost there! One comment/question I have is about the source
parameter in search_session
. Don't we still need a source_cls
parameter?
Alternatively, pass sources as objects. Your code (lines 347f) seems to already assume they are.
Does my last commit resolve you last question? |
Several improvements to the TimedBeliefDBMixin, including: