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

4 Data modalities, python typings, python testing and CLI, client multithreading for send function #18

Merged
merged 77 commits into from
Oct 24, 2024

Conversation

legoeruro
Copy link
Contributor

No description provided.

`poetry.lock` is recommended to be tracked for binary packages to ensure reproducibility, and is more commonly ignored by libraries
Also to prevents random changes in other projects from blocking development
Copy link
Member

@YiqinZhao YiqinZhao left a comment

Choose a reason for hiding this comment

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

See the comments

@@ -12,8 +12,11 @@
},
"editor.defaultFormatter": "charliermarsh.ruff"
},
"python.analysis.typeCheckingMode": "off", // TODO: Enable strict type checking
"python.analysis.typeCheckingMode": "strict",
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,9 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

I would like to keep script files in a scripts folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the suggestion! I prefer keeping build scripts colocated with their inputs for easier maintenance and to ensure they're always in sync. Do you have any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is anything right or wrong about this. I prefer to put scripts in their folders because I can open the folder and immediately know what to do in the command line with the associated files. This works better if the codebase is large, and I'm more used to this way. I don't have a strong preference for this project, we could just align the existing practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'll do that. I don't have a strong preference on this either

Copy link
Member

Choose a reason for hiding this comment

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

Do we need this file to be included in the git repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I'll add gitignores for these files. Thank you for the note

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we want to include the XR simulator assets. It would help us to decide if we can find a reference project.

// Network, device image, camera intrinsics
Debug.LogError(e);
}
Debug.Log(response.Uid);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not have Debug.Log in our core library. This logging call is slowing down the overall performance quite a bit. Maybe we can set up a verbose parameter or something to enable/disable the logging based on needs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

Copy link
Member

@YiqinZhao YiqinZhao left a comment

Choose a reason for hiding this comment

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

LGTM

@YiqinZhao YiqinZhao merged commit 2df7421 into cake-lab:main Oct 24, 2024
1 check passed
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