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
Issue 385 CLI: show beliefs does not handle NaN well #516
Conversation
…different units Signed-off-by: F.N. Claessen <felix@seita.nl>
…pling Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Pull Request Test Coverage Report for Build 3350902973
💛 - Coveralls |
Signed-off-by: F.N. Claessen <felix@seita.nl>
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.
Great. Two comments you can consider.
@@ -333,3 +335,15 @@ def duration_isoformat(duration: timedelta): | |||
# at least one component has to be there. | |||
repl = ret and "".join(ret) or "T0H" | |||
return re.sub("%P", repl, "P%P") | |||
|
|||
|
|||
def determine_minimum_resampling_resolution( |
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.
@@ -50,7 +50,7 @@ marshmallow>=3 | |||
marshmallow-polyfield | |||
marshmallow-sqlalchemy>=0.23.1 | |||
webargs | |||
uniplot | |||
uniplot>=0.7.0 |
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 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 comment
The 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 setup.py
(version 0.7.0):
python_requires=">=3.5",
install_requires=["numpy>=1.15.0"],
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.
Signed-off-by: F.N. Claessen <felix@seita.nl>
Closes #385.