pyCGM icon indicating copy to clipboard operation
pyCGM copied to clipboard

loadC3D in pyCGM_Single/pyCGMIO.py does not reset the mydictunlabeled dictionary.

Open niravkin opened this issue 5 years ago • 5 comments

https://github.com/cadop/pyCGM/blob/360fb860ce5e8e9c66f199ef8f01c06e1013bd49/pyCGM_Single/pycgmIO.py#L133

In the above loop, the dictionary mydict is set to an empty dictionary to prepare it for the next frame in the loop, but the dictionary mydictunlabeled is not. This causes unlabeled data to load incorrectly when using loadC3D.

niravkin avatar Oct 24 '20 15:10 niravkin

Can you clarify what the problem is? What is being not loaded correctly?

cadop avatar Oct 26 '20 18:10 cadop

When I use loadC3D without resetting the mydictunlabeled dictionary first, the values for unlabeled frames are not correct. If I change the function to not split the data into labeled and unlabeled data, I can get accurately loaded data for all frames. I compared this to the output I get from splitting the data into the two dictionaries, and the data for the labeled dictionaries matches, but the data for the unlabeled dictionary does not. The problem is fixed when I add this line at the end of the for loop: mydictunlabeled = {}

niravkin avatar Oct 26 '20 18:10 niravkin

Can you post a snippet of the input and output/expected output. I see the problem you are saying, but want to make sure we didn't have a reason to do that.

cadop avatar Oct 26 '20 18:10 cadop

Here is a snippet of the code I am using for testing this bug. loadC3D is the function as it exists right now. loadC3DModified is the function where the data and unlabeled data dictionaries are combined, and loadC3DFixed is the function with the proposed change. I am using Sample_Static.c3d for testing. loadC3D returns data in the format: [data,dataunlabeled,markers], so I access index 0 in modifiedc3dresults since I have combined data and unlabeled data. I am testing for the value for the unlabeled data with the name *112 in frame 0.

filename = 'Sample_Static.c3d'
modifiedc3dresults = loadC3DModified(filename) #combined data and unlabeled
oldc3dresults = loadC3D(filename)
newc3dresults = loadC3DFixed(filename)
print('Actual c3d file value:', modifiedc3dresults[0][0]['*112'])
print('Old loadC3D value:', oldc3dresults[1][0]['*112'])
print('Fixed loadC3D value:', newc3dresults[1][0]['*112'])

It produces this output: Actual c3d file value: [-225.52651978 405.57913208 1214.45861816] Old loadC3D value: [-225.48298645 400.69299316 1209.33276367] Fixed loadC3D value: [-225.52651978 405.57913208 1214.45861816]

niravkin avatar Oct 26 '20 19:10 niravkin

Okay now I see it. Good find. I suspect the issue is the dictionary added to the list is maintaining a reference to it and is being modified each loop, which modifies an earlier value.

Please make a PR with your fix and label it to close this issue.

cadop avatar Oct 26 '20 19:10 cadop