Skip to content

Implement SlidingWindow#8

Merged
sanketsabharwal merged 2 commits intomainfrom
develop/sliding_window
Aug 12, 2025
Merged

Implement SlidingWindow#8
sanketsabharwal merged 2 commits intomainfrom
develop/sliding_window

Conversation

@nicola-corbellini
Copy link
Copy Markdown
Member

This solves #6

I've also refactored Smoothness and Synchronization and related examples to use the new implemented class with the idea to show usage examples.
I'm not sure my usage is the best one, so we can discuss other options. Eventually, I can revert those changes and merge only the sliding_window.py file

Comment on lines +37 to +39
def to_array(self) -> tuple[np.ndarray, np.ndarray]:
indices = (self._start + np.arange(self._size)) % self._max_length
return self._buffer[indices], self._timestamp[indices]
Copy link
Copy Markdown
Member Author

@nicola-corbellini nicola-corbellini Aug 7, 2025

Choose a reason for hiding this comment

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

Here the code always returns a copy of the array because when the window is full, new data are written at the beginning of the arrays and the values must be reordered.
Returning a view of the array would be more memory efficient, but I haven't found a better way to implement the FIFO logic without wrapping the values

@nicola-corbellini nicola-corbellini marked this pull request as ready for review August 11, 2025 07:19
Copy link
Copy Markdown
Member

@sanketsabharwal sanketsabharwal left a comment

Choose a reason for hiding this comment

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

hi @nicola-corbellini have shared comments - will be helpful if they can be addressed.

# Add project root to import path
if os.getcwd() not in sys.path:
sys.path.append(os.getcwd())
sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), '..')))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is redundant. Not required.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep I know but it doesn't work on my machine without it. Don't understand why

Comment thread core/data_models/sliding_window.py Outdated
Comment thread core/data_models/sliding_window.py Outdated
Comment thread core/data_models/sliding_window.py Outdated
Comment thread core/data_models/sliding_window.py Outdated
Comment thread core/data_models/sliding_window.py Outdated
@nicola-corbellini
Copy link
Copy Markdown
Member Author

Hi @sanketsabharwal, addressed all the changes.

For the moment I left the sys.path append 'cause I have problems without it. Maybe I can remove the previous append.

Also, I have the doubt that type checking in the append method is the best practice if we plan to call it multiple times at high speed, but it's just a question, I don't know the answer.

Copy link
Copy Markdown
Member

@sanketsabharwal sanketsabharwal left a comment

Choose a reason for hiding this comment

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

ok tested it! works find now. about the part concerning sys, i'll figure it out when we meet next. i'll write to separately on WA to decide when we can meet for the .toe integration.

@sanketsabharwal sanketsabharwal merged commit 6de8872 into main Aug 12, 2025
@nicola-corbellini nicola-corbellini deleted the develop/sliding_window branch September 19, 2025 12:20
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.

2 participants