Kinematics tutorial - Githubissues

sfmig commented 3 months ago

Adding a kinematics tutorial!

I think I got a bit carried away, but hopefully it showcases well the possibilities ✨

Description

What is this PR

[ ] Bug fix
[ ] Addition of a new feature
[x] Other

Why is this PR needed? We want a demo of the implemented kinematic functionalities.

What does this PR do? Adds a notebook that renders in the sphinx docs, and shows how to compute the basic kinematic variables.

References

Please reference any existing issues/PRs that relate to this PR.

How has this PR been tested?

Please explain how any new code has been tested, and how you have ensured that no existing functionality has changed.

Is this a breaking change?

If this PR breaks any existing functionality, please explain how and why.

Does this PR require an update to the documentation?

If any features have changed, or have been added. Please explain how the documentation has been updated.

Checklist:

[ ] The code has been tested locally
[ ] Tests have been added to cover all new functionality
[ ] The documentation has been updated to reflect any changes
[ ] The code has been formatted with pre-commit

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.28%. Comparing base (11b5258) to head (4b7d31e).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #135 +/- ## ======================================= Coverage 99.28% 99.28% ======================================= Files 9 9 Lines 556 556 ======================================= Hits 552 552 Misses 4 4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

niksirbi commented 3 months ago

@sfmig I'd suggest rebasing to main before you proceed. We recently merged some PRs that are relevant for this one:

142
143

I've also updated the section of the contributing guide about adding examples.

sfmig commented 3 months ago

thanks @niksirbi for the feedback!

I think I addressed all your points, but let me know if I missed something:

changed to the accessor syntax,
removed the circle fit example, and
clarified the deal with the displacement vector.

Re the last point, I think this reveals that the definition of the displacement may be convenient for the developers (it's just the diff function for an array) but a bit cumbersome for the users. I would suggest to change the way we define it to point to the position at t+1 instead. Good thing is that it is an easy fix. I captured it in this issue for now #136, but happy to discuss.