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

Windows/WindowsIterator/WindowsDataset #2338

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

NicoZweifel
Copy link
Contributor

@NicoZweifel NicoZweifel commented Oct 6, 2024

Checklist

  • Confirmed that run-checks all script has been executed.
  • Made sure the book is up to date with changes in this PR.

Related Issues/PRs

Changes

  • Adds a Window trait for all Dataset<I> Instances.
  • Adds a Windows trait for all Dataset<I> Instances to keep base.rs clean.
  • Adds a proper WindowsDataset.
  • Changes windows function to return an Iterator because of the lifetime.

Testing

cd crates/burn-dataset && cargo test window

Notes

Technically the windows function functionality could also be achieved by doing sth like:

 let iter = WindowsDataset::new(dataset, 3).iter();

instead of

 let iter = dataset.windows(3);

That being said, I could imagine use-cases where the windows function is used and it would also be kinda neat if you could collect a Dataset from a WindowsIterator again, but with the current lifetime architecture I was not able to achieve this yet and it is probably more a "nice to have" than anything else.

@NicoZweifel NicoZweifel changed the title Windows/WindowIterator/WindowDataset Windows/WindowsIterator/WindowsDataset Oct 6, 2024
Copy link
Contributor Author

@NicoZweifel NicoZweifel left a comment

Choose a reason for hiding this comment

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

I'm happy with how it looks and works now.

where
D:,
{
let size = NonZeroUsize::new(size).expect("window size must be non-zero");
Copy link
Contributor Author

@NicoZweifel NicoZweifel Oct 6, 2024

Choose a reason for hiding this comment

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

I considered not panicking here and passing a NonZeroUsize instead but I came to the conclusion that not having to declare NonZeroUsize::new(x) and expect("") or unwrap() every single time is worth panicking in the constructor.

If you want to avoid panicking in the constructor, I can update the PR accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

Nan it's fine.

@NicoZweifel NicoZweifel marked this pull request as ready for review October 6, 2024 22:37
Copy link

codecov bot commented Oct 6, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 85.35%. Comparing base (dbd577a) to head (fc3cdbc).

Files with missing lines Patch % Lines
crates/burn-dataset/src/transform/window.rs 87.50% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2338      +/-   ##
==========================================
- Coverage   85.36%   85.35%   -0.01%     
==========================================
  Files         769      769              
  Lines       98738    98765      +27     
==========================================
+ Hits        84285    84305      +20     
- Misses      14453    14460       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@laggui
Copy link
Member

laggui commented Oct 9, 2024

Hey! Thanks for following up on this.

I should have time to look at your PR before the end of this week 🙂

Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for @laggui before merging.

where
D:,
{
let size = NonZeroUsize::new(size).expect("window size must be non-zero");
Copy link
Member

Choose a reason for hiding this comment

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

Nan it's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants