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

Drop the .move accessor #319

Closed
niksirbi opened this issue Oct 8, 2024 · 0 comments · Fixed by #322
Closed

Drop the .move accessor #319

niksirbi opened this issue Oct 8, 2024 · 0 comments · Fixed by #322
Labels
enhancement New optional feature

Comments

@niksirbi
Copy link
Member

niksirbi commented Oct 8, 2024

The problem

We currently have two ways of applying kinematics and filtering operations:

  1. The "functional" way: velocity = compute_velocity(ds.position)
  2. The "accessor" way: velocity = ds.move.compute_velocity()

For more information on the accessor approach, refer to the relevant section of our documentation.

This dual approach leads to several issues:

  • There are two methods to achieve the same result, both of which need to be tested, documented, and maintained.
  • It’s difficult to explain the accessor method (i.e., the .move keyword) to new users, especially those unfamiliar with object-oriented programming.
  • New contributors may find it challenging to understand what needs to be modified when adding a new filter or kinematic variable.

Proposed solution

Completely remove the accessor:

  • Delete the corresponding module and tests.
  • Remove all mentions of the accessor from the documentation.
  • Update all examples to use the functional syntax.

Alternatives

  • Maintain the status quo: This avoids a breaking change, but it might continue to confuse users and increase maintenance costs.
  • Keep the accessor but do not document it: This is the worst of both worlds, as it retains the maintenance burden without providing value to users.

Additional considerations

Things we will "lose" if we remove the accessor:

During our last meeting, we agreed that these benefits do not justify the added maintenance burden.

I'll leave this open for ~ 1 week, if no one objects, we will start the removal process.

@niksirbi niksirbi added the enhancement New optional feature label Oct 8, 2024
@niksirbi niksirbi mentioned this issue Oct 16, 2024
14 tasks
@niksirbi niksirbi moved this from 🤔 Triage to 🚧 In Progress in movement progress tracker Oct 16, 2024
@github-project-automation github-project-automation bot moved this from 🚧 In Progress to ✅ Done in movement progress tracker Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New optional feature
Projects
Development

Successfully merging a pull request may close this issue.

1 participant