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

feat: add dgp2wicker conversion #132

Merged
merged 2 commits into from
Oct 17, 2022

Conversation

chrisochoatri
Copy link
Collaborator

@chrisochoatri chrisochoatri commented Oct 7, 2022

This adds a semi standalone docker for saving DGP Synchronized Scene output in Wicker.

This is tested primarily manually due to the need for s3 access. Please build the docker, pass s3 credentials, and run test.py for more details.


This change is Reviewable

@chrisochoatri chrisochoatri added the WIP Working in progress label Oct 7, 2022
@chrisochoatri chrisochoatri force-pushed the dgp2wicker branch 4 times, most recently from 51ab0dd to c0a2a0d Compare October 11, 2022 19:21
@chrisochoatri chrisochoatri force-pushed the dgp2wicker branch 2 times, most recently from d81e0d7 to 419b291 Compare October 11, 2022 19:32
@chrisochoatri chrisochoatri removed the WIP Working in progress label Oct 11, 2022
@chrisochoatri chrisochoatri force-pushed the dgp2wicker branch 2 times, most recently from e187bd6 to 23a23ca Compare October 12, 2022 00:11
Copy link
Collaborator

@kuanleetri kuanleetri left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 16 files at r1.
Reviewable status: 11 of 16 files reviewed, 4 unresolved discussions (waiting on @chrisochoatri)


dgp/contribs/dgp2wicker/Dockerfile line 1 at r1 (raw file):

FROM dgp:latest

copyright


dgp/contribs/dgp2wicker/sample_wickerconfig.json line 1 at r1 (raw file):

{

copyright


dgp/contribs/dgp2wicker/dgp2wicker/cli.py line 117 at r1 (raw file):

    )

    print(results)

wait, just print?


dgp/contribs/dgp2wicker/dgp2wicker/dataset.py line 224 at r1 (raw file):

        """
        wicker_samples = self.wicker_sample_index[index]
        idx = wicker_samples[0]

I assume this shouldn't always get 0?
If yes, put a comment here to describe what's inside wicker_samples (or why 0)


dgp/contribs/dgp2wicker/dgp2wicker/ingest.py line 351 at r1 (raw file):

        Number of partitions to shuffle all samples over. If none, defaults to num_scenes*5

    is_pd: bool, default=False

btw I don't like this way but it seems we don't have better way lol

Copy link
Collaborator Author

@chrisochoatri chrisochoatri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 16 files reviewed, 4 unresolved discussions (waiting on @kuanleetri)


dgp/contribs/dgp2wicker/sample_wickerconfig.json line 1 at r1 (raw file):

Previously, kuanleetri (Kuan-Hui Lee) wrote…

copyright

can't, no comments in json file. This is just intended as an example wicker config that the users can update with their own s3 path.


dgp/contribs/dgp2wicker/dgp2wicker/cli.py line 117 at r1 (raw file):

Previously, kuanleetri (Kuan-Hui Lee) wrote…

wait, just print?

This will print something like {'train' 50000, 'test':1000} etc, should I add more verbose output?


dgp/contribs/dgp2wicker/dgp2wicker/dataset.py line 224 at r1 (raw file):

Previously, kuanleetri (Kuan-Hui Lee) wrote…

I assume this shouldn't always get 0?
If yes, put a comment here to describe what's inside wicker_samples (or why 0)

good catch. This was a typo left over from testing, I removed it.


dgp/contribs/dgp2wicker/dgp2wicker/ingest.py line 351 at r1 (raw file):

Previously, kuanleetri (Kuan-Hui Lee) wrote…

btw I don't like this way but it seems we don't have better way lol

I agree, not ideal...

Copy link
Collaborator Author

@chrisochoatri chrisochoatri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 16 files reviewed, 4 unresolved discussions (waiting on @kuanleetri)


dgp/contribs/dgp2wicker/Dockerfile line 1 at r1 (raw file):

Previously, kuanleetri (Kuan-Hui Lee) wrote…

copyright

Done.

Copy link
Collaborator

@kuanleetri kuanleetri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 16 files at r1, 2 of 2 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @chrisochoatri)

@chrisochoatri chrisochoatri merged commit fa3aaa2 into TRI-ML:master Oct 17, 2022
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