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

Parsing valid c3d file fails #44

Closed
yochaytz opened this issue Mar 9, 2022 · 5 comments · Fixed by #45
Closed

Parsing valid c3d file fails #44

yochaytz opened this issue Mar 9, 2022 · 5 comments · Fixed by #45

Comments

@yochaytz
Copy link
Contributor

yochaytz commented Mar 9, 2022

p2s1.zip
While trying to parse this file I get
KeyError: 'PROCESSING'
It seems that the group named PROCESSISNG appears twice in the header. Is the invalid according to the c3d format?

@AKuederle
Copy link
Collaborator

Could you provide the full trace back? And I am not sure, if that is valid based on the c3d format. @MattiasFredriksson Do you know if pyc3d assumes/required the group names to be unique?

@yochaytz
Copy link
Contributor Author

yochaytz commented Mar 9, 2022

I checked and it seems to be invalid, see here at page 36 "Each group name must be unique":
https://www.c3d.org/docs/C3D_User_Guide.pdf
Unfortunately in my case it seems that same group name was used with different group ids. I would try to work around that.
Full trace:
File "/mnt/Data_Collection_syn7/GaitAnalysis/internal/ext_datasets/GPJATK/tools/py-c3d/py-c3d/c3d/c3d.py", line 1440, in init
self.add_group(group_id, name, desc)
File "/mnt/Data_Collection_syn7/GaitAnalysis/internal/ext_datasets/GPJATK/tools/py-c3d/py-c3d/c3d/c3d.py", line 1118, in add_group
raise KeyError(name)
KeyError: 'PROCESSING'
python-BaseException

@yochaytz
Copy link
Contributor Author

yochaytz commented Mar 9, 2022

Eventually I modified add_group() such that if group name is already exist, it auto-generates different name ('PROCESSING' --> 'PROCESSING1'). Files are now readable.

@AKuederle
Copy link
Collaborator

Great! Could you share the modifications you made? Ideally as a merge request. Then we can discuss, if we want to add this workaround as well.
If you need any help with opening a merge request, I can help you.

@MattiasFredriksson
Copy link
Contributor

MattiasFredriksson commented Mar 9, 2022

Interesting case where the file contains two GROUP entries with the same name. I think supporting such cases would be good as long as a warning message is registered when it occurs as to inform users when the file does not follow the standard. It would also be good to follow the 'parameter naming convention' when generating the names, namely: PROCESSING, PROCESSING2, PROCESSING3... I'm not sure every file would follow that naming convention but it would be good to stick with a similar convention as the examples in the manual (see 'Additional Parameter' section).

@AKuederle AKuederle linked a pull request Mar 10, 2022 that will close this issue
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 a pull request may close this issue.

3 participants