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

Improve comments and variable names #44

Open
jf514 opened this issue Aug 27, 2024 · 1 comment
Open

Improve comments and variable names #44

jf514 opened this issue Aug 27, 2024 · 1 comment

Comments

@jf514
Copy link
Contributor

jf514 commented Aug 27, 2024

Some improvements to comments, etc.

  1. Explain why mjx data (comment about creating on GPU): https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/controller.py#L201
  2. Change _lb and _ub to _joint_lower_bound and _joint_upper_bounds https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/controller.py#L79
  3. Fix comment (quaternion 4): https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/controller.py#L27
  4. Change n to more descriptive variable name: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/controller.py#L62C8-L64C10
  5. Doc string out of date: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/controller.py#L111-L118
  6. Remove commented field #phyisics: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/controller.py#L159
  7. Comment as to why regulariation is here or move to it's own function: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/controller.py#L151-L156
  8. Comment or more descriptive names or both: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/controller.py#L71-L72
  9. Clarify or enumerate "and such": https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/controller.py#L210
  10. What do we mean by 'optimize`: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/compute_stac.py#L25
  11. Change to "returns an mjx_data updated such that..."
  12. Explain magic number 7: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/compute_stac.py#L54
  13. Change this function to just take a single frame: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/compute_stac.py#L16
  14. Change 'res' to residual: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/compute_stac.py#L57
  15. Change 'update' to 'run inverse kinematics', and add argument parameters: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/stac_base.py#L86C8-L86C15
  16. Change output to say 'q_opt 2 finished in ...' https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/compute_stac.py#L95
  17. Change to run_kinematics: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/operations.py#L12
  18. Set default frame parameter to make it clear this only happening on first frame: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/controller.py#L222
  19. Change to 'Optimize marker offsets such that...': https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/compute_stac.py#L117
  20. Change to 'updated such that...': https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/compute_stac.py#L131
  21. Change m_opt to marker_optimization: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/stac_base.py#L186
  22. Clarify 'phase optimization': https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/stac_base.py#L197
  23. Fill out these values: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/stac_base.py#L200-L207
  24. Change name to sample_frame_indices: https://github.com/talmolab/stac-mjx/blob/main/stac_mjx/compute_stac.py#L138
  25. Add site_idxs arg to doc string:
    mjx_data (mjx.Data):
  26. Same here:
    site_idxs: jp.ndarray,
  27. Comment this line to explain that these are the new params, and possibly change variable name:
    offset_opt_param = res.params
@charles-zhng
Copy link
Collaborator

Thanks for enumerating these. Some comments on some of these:
7. This is just for defining the is_regularized mask, which is there because it uses site_index_map. Can move to new function.
12. Wrong link?
13. This can be done after we set up the workflow for testing each optimization step individually to find if the second optimization step is helpful or not.
14. res -> result. Can change to that instead
22. I think I meant to say "m_phase" here. Def need to write a real docstring for that one

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

No branches or pull requests

2 participants