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 385 CLI: show beliefs does not handle NaN well #516

Merged
merged 6 commits into from Oct 29, 2022

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Oct 21, 2022

Closes #385.

…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>
@Flix6x Flix6x added bug Something isn't working Data CLI upgrading labels Oct 21, 2022
@Flix6x Flix6x self-assigned this Oct 21, 2022
Signed-off-by: F.N. Claessen <felix@seita.nl>
@coveralls
Copy link
Collaborator

coveralls commented Oct 21, 2022

Pull Request Test Coverage Report for Build 3350902973

  • 6 of 20 (30.0%) changed or added relevant lines in 3 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.07%) to 64.949%

Changes Missing Coverage Covered Lines Changed/Added Lines %
flexmeasures/utils/time_utils.py 2 4 50.0%
flexmeasures/data/models/generic_assets.py 1 4 25.0%
flexmeasures/cli/data_show.py 3 12 25.0%
Files with Coverage Reduction New Missed Lines %
flexmeasures/init.py 2 81.82%
Totals Coverage Status
Change from base Build 3259160910: -0.07%
Covered Lines: 6514
Relevant Lines: 9413

💛 - Coveralls

@Flix6x Flix6x marked this pull request as ready for review October 21, 2022 10:44
Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x requested a review from nhoening October 21, 2022 10:52
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.

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(
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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>
@Flix6x Flix6x merged commit 1c69a0e into main Oct 29, 2022
@Flix6x Flix6x deleted the issue-385_CLI_show_beliefs_does_not_handle_NaN_well branch October 29, 2022 09:27
@Flix6x Flix6x added this to the 0.12.0 milestone Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLI Data upgrading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: show beliefs does not handle NaN well
3 participants