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 385 CLI: show beliefs does not handle NaN well #516
Changes from all commits
d381791
478e575
b3519eb
e29655a
ddad5a0
f853dfb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,8 @@ marshmallow>=3 | |
marshmallow-polyfield | ||
marshmallow-sqlalchemy>=0.23.1 | ||
webargs | ||
uniplot | ||
# Minimum version that correctly aligns time series that include NaN values | ||
uniplot>=0.7.0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't see how you use uniplot differently now. It would probably still work with <0.7.0, just not if NaN is in the data. So, do we technically need to depend on a higher version, or do we like this feature to be around? Always good to add the reasoning as a comment to make this clear for later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like this feature to be around. When selecting multiple sensors for some time period, having NaN data in that set is very common. With previous uniplot versions, the time series will not align right, which is a serious problem to me. From uniplot's
which are older versions than the first ones we started using for this repo as far back as I can tell (10cff9f). I'll add an inline note to where we define the minimum version requirement. |
||
# flask should be after all the flask plugins, because setup might find they ARE flask | ||
# Minimum here due to Flask-Classful not supporting Werkzeug 2.2.0 yet, see https://github.com/teracyhq/flask-classful/pull/145 | ||
flask>=1.0, <=2.1.2 | ||
|
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.
In my PR in flexmeasures-simulate, I have a similar util function, and I decided to let it
raise
if the resolutions are actually not compatible (if any([resolution % min_resolution != timedelta(hours=0) for resolution in resolutions]
)This function could do this, maybe triggered by a parameter flag
raise_if_incompatible
. Thoughts?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.
In my mind, no combination of resolutions should be incompatible. Resampling a (simplified) time series with instantaneous values should be rather straightforward: when downsampling, select the first value; when upsampling, interpolate using some strategy (such as forward fill or linear interpolation), which we could later define as a sensor attribute. I'll address this soon in a timely-beliefs PR.