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

Documentation and support for editing .c3d files. #33

Open
wants to merge 125 commits into
base: master
Choose a base branch
from

Conversation

MattiasFredriksson
Copy link
Contributor

@MattiasFredriksson MattiasFredriksson commented Jul 6, 2021

Major changes:

  • Added Writer.from_reader() and Reader.to_writer() methods.
  • Updated existing documentation using pdoc3 (available on github.io).
  • Refactored Reader to return ParamData decorated with ParamReadonly to prevent accidental editing of files in read mode.
  • Refactored the project into multiple files to make it easier manage in the future (I'm allergic to large files).

Minor changes:

  • Updated c3d-viewer to loop and not crash.
  • Read, Write, Read tests (checks if the data written to a file is identical).
  • Added convenience functions.
  • More...

Why?

To actually support writing files (Issue #32), last update was focused only on ensuring reading worked. In particular, the goal in this case was to support editing files (since it provides test cases for writing files). The package now support looping a file by the simple example provided in the documentation:

import c3d

with open('my-motion.c3d', 'rb') as file:
    reader = c3d.Reader(file)
    writer = reader.to_writer('copy')
    for i, points, analog in reader.read_frames():
        writer.add_frames((points, analog), index=reader.frame_count)

with open('my-looped-motion.c3d', 'wb') as h:
    writer.write(h)

Downsides

Old documentation was largely outdated so I edited/rewrote most of it, and the wording might not always be perfect.
The documentation links are linked to my github.io. However, one can easily create a project website through the github settings tab by following this guide and updating the project links (in the root and root/docs/ README).

The update should be stable, but probably not pip ready. However it seems people enjoy pip and don't update before posting issues (e.g. (Issue #15)), so I wanted to advise downloading in the docs.

I removed most default parameters in the Writer as, in principle, the user needs to set them themselves. However, some of the properties should probably be reintroduced.

There are probably way to many changes for comfort but hopefully you approve.

BR
Mattias Fredriksson

processor_convert() -> _processor_convert()
+ Manager.group_items
+ Manager.group_listed
+ int_array, uint_array, int64_array, uint64_array float32_array, float64_array

+- float_array now returns either float32 or float64 arrays.
+ int_array/uint_array return either 1/2/4/8 bit integer arrays.
----
Param.dtype -> Param._dtypes
Group.name, Group.desc -> Group._name, Group._desc

Additions
----
+ Group._dtypes
+ rename_group(), rename_param(), remove_group(), remove_param()
+ Comments
+ Code corrections
ManagerGroupTests.test_Manager_add_group
ManagerGroupTests.test_Manager_remove_group_from_numeric
ManagerGroupTests.test_Manager_remove_group_from_name
+ Added Group.values() accessor
+ Writer._dtypes
---
+ Group.param_keys()
+ Manager.group_keys()

Changed
----
Group.values() -> Group.param_values()
Group.items() -> Group.param_items()
KeyError -> ValueError for existing keys
…py') + more

Corrections:
----
Read TRIAL:ACTUAL_END_FIELD as 2 16 bit words in accordance with docs
Read POINT:LONG_FRAMES as float in accordance with docs

Set parameters helpers:
---
analog labels
point labels
analog transform
analog offset
start/end frame
+?
.. code-block:: python
.rst working with .. code-block:: python
Manager.get_screen_xy_axis
ParamReadonly.any_value()
ParamReadonly.any_array()
…ed convenience functions giving direct access to value attributes from `c3d.manager.Manager`
@MattiasFredriksson MattiasFredriksson changed the title Documentation and improved support for editing.c3d files. Documentation and improved support for editing .c3d files. Jul 6, 2021
@MattiasFredriksson
Copy link
Contributor Author

MattiasFredriksson commented Jul 6, 2021

@lmjohns3 Look it through if you have time.

@MattiasFredriksson MattiasFredriksson changed the title Documentation and improved support for editing .c3d files. Documentation and support for editing .c3d files. Jul 7, 2021
@@ -0,0 +1,22 @@
The MIT License (MIT)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The licence should remain top-level and not be moved to the c3d subfolder

@@ -0,0 +1,54 @@
Code style
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't have a second readme inside the code folder.

Is this related to the documentation tool you are using?

@@ -0,0 +1,10 @@
Tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above haveing a second nested README is unusual

@AKuederle
Copy link
Collaborator

@MattiasFredriksson I think there are a bunch of valuable contributions in there, but I think we need to split it up in to smaller change sets to make it manageable to discuss. I know that is a lot of additional work.

I had a quick look through all the files and there seem to be a couple of general topics that we should seperate:

  • Refactoring of the structure (sub folder and splitting into multiple files) <-- This is great!
  • Additional test <-- Also great
  • Fixed bug regarding naming of the scripts <- Also great
  • Adding functionality to edit files <- Great, but I think this requires some discussion/in detail code review
  • Update of the documentation tool <- My personal oppinion is that we should stick to sphinx and rtd, as this is the defacto standard in the community. The new documentation pages however are awesome.

Would it be possible to separate the first three changes from the rest, as I think they are not controversial and could be merged relatively quickly. Ideally cherry-pick the relevant commits to a new branch or if that is not possible create a new branch copy the new files over/make the required modifications again and then recommit.

Let me know if I can be of help!

@MattiasFredriksson
Copy link
Contributor Author

MattiasFredriksson commented Jan 21, 2022

Hey thanks for the feedback, can take a look on the weekend but can't promise to much progress. With regards to multiple README files it is a non-issue, can move the information out of the sub-folders, I just enjoyed a clean front page on github. Changing the documentation tool should be doable. The reason for using it was that it gave good results without any manual configuration, with more interest in package maintenance taking the time to set up another documentation tool is fine.

With regards to separating out edit functionality I don't think it is worth it as it was the purpose of making the changes in the first place. Writing files was not working well (particularly with the previous update(s)), and the remaining changes was just a result of providing a stable end result that could be further developed (e.g. document changes, working in a single file became unmanageable). It would probably be possible to do it the other way around (i.e. resolve the writing/editing part first, before documentation changes).

Some smaller changes could be factored out however such as tests, but if I don't remember incorrectly the tests was made primarily to compare read/write results and most would not pass in master for all test files (particularly when reading files in non-IEEE format).

I'll try to see what's possible though, it was some time since I looked over the code so I might make some realizations once I have time to look it over.

BR
Mattias

@friggog
Copy link
Collaborator

friggog commented Jan 21, 2022

+1 for small PRs to bring these changes in gradually with proper review

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.

3 participants