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

Form new beliefs given some model #50

Merged
merged 73 commits into from
Oct 21, 2022
Merged

Form new beliefs given some model #50

merged 73 commits into from
Oct 21, 2022

Conversation

Flix6x
Copy link
Collaborator

@Flix6x Flix6x commented Feb 8, 2021

This introduces a new BeliefsDataFrame method form_beliefs that creates new beliefs given a model, a belief_time and an event_time_window.

Much credit goes to the AI Master students from the University of Amsterdam that also helped us implement ridgeline plots for probabilistic forecasts.

topteulen and others added 30 commits June 24, 2019 14:19
* added generate function

* Update generate.py

* Create ridgeline plot

* Add example plots ridgeline

* add ridgeline examples

* add titles

* added plotting function and cleaned up code

* added plotting and cleaned up code

* code cleanup and commenting

* fixed typo's

* update + error messages

* updated error messages

* Copied ridgeline README documentation

* Added ridgeline README files

* Added generate documentation

* fixed typo

* fix labels
Distinguish how mean and median averages are calculated.
Allow additional distribution parameters to be set.
@Flix6x
Copy link
Collaborator Author

Flix6x commented Aug 24, 2022

Test setup is failing for Python 3.7, with RuntimeError: Scikit-learn requires Python 3.8 or later. The current Python version is 3.7.13. My proposal would be to make sktime (which is requiring scikit-learn) a soft dependency, installable with pip install timely-beliefs[forecast] (after merging #107), and exclude tests relating to forming new beliefs for python==3.7.

@Flix6x Flix6x marked this pull request as ready for review September 15, 2022 13:57
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.

Awesome to finally bring this in, and to start usign sktime!

I have mainly questions to offer.

timely_beliefs/tests/test_belief_formation.py Outdated Show resolved Hide resolved
timely_beliefs/beliefs/classes.py Show resolved Hide resolved
timely_beliefs/beliefs/classes.py Show resolved Hide resolved
timely_beliefs/beliefs/classes.py Show resolved Hide resolved
timely_beliefs/beliefs/classes.py Outdated Show resolved Hide resolved
freq=df.event_resolution,
closed="left",
)
forecaster.fit(df["event_value"])
Copy link
Contributor

Choose a reason for hiding this comment

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

At this time, our BeliefDataFrames do not support regressors to be used here, correct? For example, forecasting wind speed based on time of day and temperature - how would we do that? (maybe another ticket)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea to separate this into another ticket. It can be done by passing an extra X to the fit function, for example, like this:

X=df.loc[:, df.columns != "event_value"]

Here, I'm assuming the external regressors are given as additional columns in the BeliefsDataFrame.

Testing it with sktime's naive forecaster will just lead to the forecaster ignoring X though, so that doesn't feel like a wholesome test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we make an issue out of this yet? I can't find one.

Copy link
Contributor

@nhoening nhoening Oct 21, 2022

Choose a reason for hiding this comment

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

Ah, I see you already add the X in this PR. I believe a little comment would help.

And is there a case where columns in X would be bad to include?

We could also add an optional parameter to form_beliefs, e.g. "X_column_names".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created #116 just now.

@Flix6x Flix6x requested a review from nhoening October 13, 2022 09:21
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.

I have one comment/question about X.

@Flix6x Flix6x merged commit 091c08c into main Oct 21, 2022
@Flix6x Flix6x deleted the generate branch October 21, 2022 22:34
@Flix6x Flix6x added this to the 1.13.0 milestone Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants