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

Timed beliefs #79

Merged
merged 12 commits into from Apr 2, 2021
Merged

Timed beliefs #79

merged 12 commits into from Apr 2, 2021

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Mar 29, 2021

This PR introduces the TimedBelief table and functionality to turn its entries into a BeliefsDataFrame and vice versa.

As part of your review, I'd be interested to get your opinion on the naming of the class methods I introduce for this functionality.

Best reviewed in conjunction with timely-beliefs PR 53.

@Flix6x Flix6x added the Data label Mar 29, 2021
@Flix6x Flix6x requested a review from nhoening March 29, 2021 21:17
@Flix6x Flix6x self-assigned this Mar 29, 2021
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.

Good PR.

I have no super-convicing case for naming, but my personal choice would be for simple words and to use them consistently in both libraries (you used search_all versus query_all and persist_all versus add_all).

I also have some questions about the implementations of the DB mixins. Probably will need a bit of discussion.

flexmeasures/data/models/time_series.py Outdated Show resolved Hide resolved
flexmeasures/data/models/time_series.py Outdated Show resolved Hide resolved
flexmeasures/data/models/time_series.py Show resolved Hide resolved
flexmeasures/data/models/time_series.py Show resolved Hide resolved
@Flix6x
Copy link
Contributor Author

Flix6x commented Apr 1, 2021

Thanks for your thoughts regarding method naming. I suggest the following:

  • flexmeasures: TimedBelief.search() and TimedBelief.add()
  • timely-beliefs: TimedBelief.search_session() and TimedBelief.add_to_session()

This way, the naming reflects when you have to pass it a session explicitly, and the fm class method names won't overwrite the tb class method names, as they would if they are named exactly the same. Alternatively, it could be:

  • flexmeasures: TimedBelief.search() and TimedBelief.add()
  • timely-beliefs: TimedBelief._search() and TimedBelief._add()

or:

  • flexmeasures: TimedBelief.search_session() and TimedBelief.add_to_session()
  • timely-beliefs: TimedBelief._search_session() and TimedBelief._add_to_session()

But I prefer (and implemented) the first suggestion.

@Flix6x Flix6x requested a review from nhoening April 1, 2021 14:47
@Flix6x Flix6x merged commit a60ed29 into main Apr 2, 2021
@Flix6x Flix6x deleted the timed-beliefs branch April 2, 2021 18:29
@nhoening nhoening added this to the 0.4.0 milestone Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants