Skip to content
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

Issue 259 synchronize how we collect data from data models #260

Merged

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Dec 4, 2021

This PR synchronizes the function signatures of our old and new data collection methods, thereby supporting us in moving over data to our new data model. It introduces a new TimedBelief.collect method whose function signature is compatible with calls to TimedValue.collect. To ensure equivalency, some query filter utils have been refactored to become filter criteria (i.e. the SQLAlchemy term for that which you pass to a query filter), and timely-beliefs 1.8.0 is on its way to support passing additional custom criteria for its search query filters.

closes #259

@Flix6x Flix6x self-assigned this Dec 4, 2021
@Flix6x Flix6x linked an issue Dec 4, 2021 that may be closed by this pull request
2 tasks
@Flix6x
Copy link
Contributor Author

Flix6x commented Dec 4, 2021

Still requires a pip-compile --output-file=requirements/app.txt requirements/app.in after releasing timely-beliefs==1.8.0.

@Flix6x Flix6x requested a review from nhoening December 4, 2021 10:36
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but I have a problem to understand a central piece of this. We can also do a call if that would be quicker to explain.

flexmeasures/data/models/time_series.py Show resolved Hide resolved
flexmeasures/data/queries/utils.py Outdated Show resolved Hide resolved
@Flix6x Flix6x marked this pull request as ready for review December 6, 2021 11:06
@Flix6x Flix6x requested a review from nhoening December 6, 2021 13:22
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ... I might have missed it, but why do we need Sensor.search, when Sensor.collect gets the same parameters and is more powerful:

  • can query multiple sensors if needed, and sum, if needed
  • can resample

@@ -225,6 +225,8 @@ def test_query_beliefs(setup_beliefs):
source = DataSource.query.filter_by(name="Seita").one_or_none()
bdfs = [
TimedBelief.search(sensor, source=source),
TimedBelief.search(sensor.id, source=source),
TimedBelief.collect(sensor.name, source=source),
sensor.search_beliefs(source=source),
tb.BeliefsDataFrame(sensor.beliefs), # doesn't allow filtering
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are all beliefs in setup_beliefs from one source anyways? Then we are not really tresting the filtering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. We're using the filter only to test whether a FlexMeasures DataSource, which subclasses the timely-beliefs BeliefSource, results in a query that the database can still understand.

@Flix6x
Copy link
Contributor Author

Flix6x commented Dec 6, 2021

why do we need Sensor.search, when Sensor.collect gets the same parameters and is more powerful

Neither exists. Are you instead suggesting to expand the capabilities of TimedBelief.search to get rid of TimedBelief.collect? This would be possible. I'd then also rename all occurences of TimedValue.collect to become TimedValue.search, in order to safeguard the goal of this PR.

@nhoening
Copy link
Contributor

nhoening commented Dec 6, 2021

Sorry, I meant TimedBelief.search and TimedBelief.collect.

To me, your suggestion sounds like it leads to less confused questions like mine. If you also like that design and it's not too much work, go ahead.

@Flix6x Flix6x merged commit db2ea57 into project-9 Dec 6, 2021
@Flix6x Flix6x deleted the issue-259-Synchronize_how_we_collect_data_from_data_models branch December 6, 2021 14:19
@Flix6x Flix6x mentioned this pull request Dec 6, 2021
Flix6x added a commit that referenced this pull request Dec 6, 2021
Merging the new TimedBelief.collect method with the existing TimedBelief.search method. This also entails renaming all occurences of TimedValue.collect to become TimedValue.search, in order to safeguard the goal of PR #260.


* Rename old collect method to search

* Merge new collect method into TimedBelief.search

* Deprecate the sensors argument in favor of sensor

* Small annotation revert in scope of project 9

* Fix deprecation of required argument
@Flix6x Flix6x added this to the 0.8.0 milestone Jan 20, 2022
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.

Synchronize how we collect data from data models
2 participants