Skip to content

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

Merged
YiqinZhao merged 77 commits intocake-lab:mainfrom
FelixNgFender:main
Oct 24, 2024
Merged

4 Data modalities, python typings, python testing and CLI, client multithreading for send function #18
YiqinZhao merged 77 commits intocake-lab:mainfrom
FelixNgFender:main

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

"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
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