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.

Sign up for GitHub

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

Jump to bottom

Expose gaps in moving window #558

Closed
daniel-zullo-frequenz opened this issue Aug 2, 2023 · 10 comments
Closed

Expose gaps in moving window #558

daniel-zullo-frequenz opened this issue Aug 2, 2023 · 10 comments
Assignees
@cwasicki
Labels
part:data-pipeline Affects the data pipeline priority:high Address this as soon as possible type:enhancement New feature or enhancement visitble to users
Milestone
v1.0.0-rc2

Comments

@daniel-zullo-frequenz
Copy link
Contributor

daniel-zullo-frequenz commented Aug 2, 2023 •
edited

What's needed?

Implement method to return the maximum data gap in a MovingWindow given timestamp or index ranges representing a window size

Proposed solution

Compute the maximum data gap size iterating through the MovingWindow data gaps (underlying buffer) for the given timestamp or index ranges representing a window size

Use cases

Actors like peak-shaving needs this functionality to avoid implementing it on their ends.

Update

Note the scope of the issue was updated as follows:
The MovingWindow will provide access to a list of all gaps in a defined subset of the moving window. Applications can get the maximum gap or the sum of gaps by using something like max(g.duration for g in gaps) or sum(...), respectively.
@daniel-zullo-frequenz daniel-zullo-frequenz added part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed type:enhancement New feature or enhancement visitble to users labels Aug 2, 2023
@daniel-zullo-frequenz
Copy link
Contributor Author

fyi @matthias-wende-frequenz @idlir-shkurti-frequenz

Please feel free to amend/correct the issue description accordingly

@matthias-wende-frequenz matthias-wende-frequenz added this to the v0.24.0 milestone Aug 2, 2023
@matthias-wende-frequenz matthias-wende-frequenz added priority:high Address this as soon as possible part:data-pipeline Affects the data pipeline and removed part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed labels Aug 2, 2023
@llucax llucax modified the milestones: v0.24.0, v0.25.0 Aug 9, 2023
shsms added a commit to shsms/frequenz-sdk-python that referenced this issue Aug 21, 2023
@shsms
Bump pytest-cov from 4.0.0 to 4.1.0 (frequenz-floss#406)
776352c 
Bumps [pytest-cov](https://github.com/pytest-dev/pytest-cov) from 4.0.0
to 4.1.0.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/pytest-dev/pytest-cov/blob/master/CHANGELOG.rst">pytest-cov's
changelog</a>.</em></p>
<blockquote>
<h2>4.1.0 (2023-05-24)</h2>
<ul>
<li>Updated CI with new Pythons and dependencies.</li>
<li>Removed rsyncdir support. This makes pytest-cov compatible with
xdist 3.0.
Contributed by Sorin Sbarnea in
<code>[frequenz-floss#558](pytest-dev/pytest-cov#558)
&lt;https://github.com/pytest-dev/pytest-cov/pull/558&gt;</code>_.</li>
<li>Optimized summary generation to not be performed if no reporting is
active (for example,
when <code>--cov-report=''</code> is used without
<code>--cov-fail-under</code>).
Contributed by Jonathan Stewmon in
<code>[frequenz-floss#589](pytest-dev/pytest-cov#589)
&lt;https://github.com/pytest-dev/pytest-cov/pull/589&gt;</code>_.</li>
<li>Added support for JSON reporting.
Contributed by Matthew Gamble in
<code>[frequenz-floss#582](pytest-dev/pytest-cov#582)
&lt;https://github.com/pytest-dev/pytest-cov/pull/582&gt;</code>_.</li>
<li>Refactored code to use f-strings.
Contributed by Mark Mayo in
<code>[frequenz-floss#572](pytest-dev/pytest-cov#572)
&lt;https://github.com/pytest-dev/pytest-cov/pull/572&gt;</code>_.</li>
<li>Fixed a skip in the test suite for some old xdist.
Contributed by a bunch of people in
<code>[frequenz-floss#565](pytest-dev/pytest-cov#565)
&lt;https://github.com/pytest-dev/pytest-cov/pull/565&gt;</code>_.</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/2c9f2170d8575b21bafb6402eb30ca7de31e20b9"><code>2c9f217</code></a>
Bump version: 4.0.0 → 4.1.0</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/4d245df8f75e434a9e1b162b51265d6a45017465"><code>4d245df</code></a>
Update changelog and authors.</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/7b095c84ae521b14058d7d520ef36372849063a8"><code>7b095c8</code></a>
Skip starting from xdist 3.0.2 (where boxed was removed).</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/605d6902b3b3d17aad0bf811b8c580fc895724ca"><code>605d690</code></a>
disabling boxed test if version xdist newer than 2.5.0</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/76fb2a6cb2c4a4a788a5b62710848daf9c8fb7ce"><code>76fb2a6</code></a>
introduced f-strings</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/0d63ede0d2ca9f4acc8329aa4261a7cec489ffdb"><code>0d63ede</code></a>
Update test config. Reapply some of the changes from PR567 to the right
file ...</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/f3d8d8380f6a4b265353fe7cd509b040702f1e64"><code>f3d8d83</code></a>
Add support for JSON reporter</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/dec02abeb9fa8ee3547baa054bde6006bea530ee"><code>dec02ab</code></a>
Update test deps.</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/88a7d348986bace58e26c88a713ef35f900ce2ef"><code>88a7d34</code></a>
chore: update AUTHORS and CHANGELOG</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/74eb4cc8b684269b89735e31b623f0f9795c5d5c"><code>74eb4cc</code></a>
perf: only call summary when the report will be used</li>
<li>Additional commits viewable in <a
href="https://github.com/pytest-dev/pytest-cov/compare/v4.0.0...v4.1.0">compare
view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pytest-cov&package-manager=pip&previous-version=4.0.0&new-version=4.1.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)

</details>
@llucax llucax modified the milestones: v0.25.0, v1.0.0-rc Aug 22, 2023
@cwasicki
Copy link
Collaborator

After discussion with @idlir-shkurti-frequenz we change the scope of this issue. The max gap is currently used in peak shaving but is going to be replaced with a sum of gaps soon. Therefore we plan to provide access to a list of all gaps in a defined subset of the moving window. By this current and future requirements can be achieved using something like max(g.duration for g in gaps) or sum(...), respectively.

@llucax
Copy link
Contributor

llucax commented Sep 5, 2023

If there is a PR for this, can you link it to the issue @cwasicki ?

@cwasicki
Copy link
Collaborator

cwasicki commented Sep 5, 2023

Since it's not a breaking change I think we can move it post v1.

@llucax
Copy link
Contributor

llucax commented Sep 6, 2023

@daniel-zullo-frequenz said he thought there was a PR for this, so I guess this is not the case, right?

@cwasicki
Copy link
Collaborator

cwasicki commented Sep 6, 2023

I have some changes locally which I can push and link

@cwasicki cwasicki mentioned this issue Sep 6, 2023
Closed
@cwasicki
Copy link
Collaborator

cwasicki commented Sep 6, 2023

#643

@cwasicki
Copy link
Collaborator

cwasicki commented Sep 6, 2023

@daniel-zullo-frequenz Could you update title and description to something like Expose gaps in moving window (see #558 (comment))

@daniel-zullo-frequenz daniel-zullo-frequenz changed the title Get maximum data gap size Expose gaps in moving window Sep 7, 2023
@daniel-zullo-frequenz
Copy link
Contributor Author

done

@llucax llucax modified the milestones: v1.0.0-rc1, v1.0.0-rc2 Sep 12, 2023
@cwasicki cwasicki mentioned this issue Sep 13, 2023
Closed
@cwasicki
Copy link
Collaborator

cwasicki commented Oct 2, 2023

Discussed with @idlir-shkurti-frequenz that this can be closed as they can use the new moving window functions

frequenz-sdk-python/src/frequenz/sdk/timeseries/_moving_window.py

Line 357 in a06db70

def count_valid(self) -> int:

and

frequenz-sdk-python/src/frequenz/sdk/timeseries/_moving_window.py

Line 366 in a06db70

def count_covered(self) -> int:

to determine the data quality of the window in peak shaving.

@cwasicki cwasicki closed this as completed Oct 2, 2023
@cwasicki cwasicki mentioned this issue Oct 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cwasicki cwasicki

Labels
part:data-pipeline Affects the data pipeline priority:high Address this as soon as possible type:enhancement New feature or enhancement visitble to users
Projects
Python SDK Roadmap
Status: Done
Milestone
v1.0.0-rc2
Development

Successfully merging a pull request may close this issue.

Add duration to gaps and expose gaps in moving window cwasicki/frequenz-sdk-python
4 participants
@llucax @matthias-wende-frequenz @daniel-zullo-frequenz @cwasicki