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

Fixed the parser for DReyeVR actor data #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

asarunova
Copy link

Fixed the parser for actor data - made it more readable and compatible with panda data frames.

actors_df = convert_to_df(actors_data)

# Display the frame
from IPython.display import display
Copy link
Collaborator

Choose a reason for hiding this comment

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

This display variable is unused in the rest of the program.

Also I'd recommend to move all your imports to the top of the file for better readability

)
args = argparser.parse_args()

main(args.file, args.out)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would also be nice to include an example log (txt) file that someone can run using this simple test just to see what the outputs look like

@@ -239,6 +241,9 @@ def parse_file(

actors_key: str = "Actors"
data[actors_key] = {}
data[actors_key]["Id"] = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a fine change, but as is you'll need to fix the example.py script (Line ~470) which still uses the "Id" method to index into a dictionary containing "Location" and "Rotation". Can you please fix that?

assert min(lens) == max(lens) # all lengths are equal!
# NOTE: pandas can't haneld high dimensional np arrays, so we just use lists
# all lengths are equal!
if not min(lens) == max(lens):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what the benefit of this is over a simple assert. Requires users to have the ipdb module loaded and will issue a nasty ModuleNotFoundError which might confuse the users.

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.

2 participants