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

Do we need a validate() method in move accessor? #257

Closed
sfmig opened this issue Aug 5, 2024 · 3 comments · Fixed by #322
Closed

Do we need a validate() method in move accessor? #257

sfmig opened this issue Aug 5, 2024 · 3 comments · Fixed by #322
Labels
enhancement New optional feature

Comments

@sfmig
Copy link
Contributor

sfmig commented Aug 5, 2024

Is your feature request related to a problem? Please describe.
While reviewing PR #255, @lochhh pointed out that with a poses datasets we only use the move.validate() method when saving the dataset to an output file.

The PR extends the move accessor to bboxes datasets, including the move.validate() method, and adds a test to cover the case of validating a bboxes dataset (since we don't have an equivalent save_bboxes module yet).

This raised the question: do we need this method?

Describe the solution you'd like
For now extending the existing accessor methods and corresponding tests makes sense to me, but we may want to consider if this is necessary and if it will be a burden going forwards.

Describe alternatives you've considered
\

Additional context
\

@sfmig sfmig added the enhancement New optional feature label Aug 5, 2024
@lochhh
Copy link
Collaborator

lochhh commented Aug 5, 2024

Note also that we have validator modules - could we (as with other move methods, e.g. compute kinematics) use any of these in move.validate?

@niksirbi
Copy link
Member

We could get rid of it in principle. As stated above, we only use it in one place (prior to saving).

The original idea was to use validate() in many places, e.g. before computing derivatives. That said, as movement has become more flexible we tend to do more specific validations depending on what a specific function/method needs, so the case for a general dataset validation method has weakened.

Under the hood the validate() method checks for the presence of expected dimensions and data variable before forwarding the rest of the checks to ValidPosesDataset or ValidBboxesDataset.

self._validate_dims()
self._validate_data_vars()

If we were to remove validate(), we should find another place for doing these checks (perhaps in the save_poses.py module itself?).

@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
@niksirbi
Copy link
Member

niksirbi commented Oct 16, 2024

We now are getting rid of this validation as part of #322.
The functionalities of self._validate_dims() and self._validate_data_vars() are being transferred to the save_poses.py module, i.e. where they are actually being used. The calls to ValidPosesDataset and ValidBboxesDatasetprior to saving don't happen anymore, they were too restrictive in my opinion.

@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.

3 participants