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

Lberg/doc ref systems #66

Merged
merged 13 commits into from
Aug 21, 2020
Merged

Lberg/doc ref systems #66

merged 13 commits into from
Aug 21, 2020

Conversation

lucabergamini
Copy link
Contributor

PR to add a doc section about the different reference systems in L5Kit

coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
oliver-scheel
oliver-scheel previously approved these changes Jul 20, 2020
Copy link
Contributor

@oliver-scheel oliver-scheel left a comment

Choose a reason for hiding this comment

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

Minors, otherwise LGTM
Satellite and Semantic still to be done.

@yawei-ye
Copy link
Contributor

Minors, otherwise LGTM
Satellite and Semantic still to be done.

Do not forget to approve the PR when you think it looks good to you xD

yawei-ye
yawei-ye previously approved these changes Jul 24, 2020
Copy link
Contributor

@yawei-ye yawei-ye left a comment

Choose a reason for hiding this comment

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

I pushed a minor change, otherwise LGTM

oliver-scheel
oliver-scheel previously approved these changes Jul 24, 2020
- satellite ref system description
- semantic ref system description
- links for ref systems (ECEF and ENU)
coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
draw_trajectory(im, positions_pixels, data["target_yaws"], TARGET_POINTS_COLOR)
```

## Satellite Coordinate System
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd clarify this section a bit, as three coordinate systems are used (image, world, ECEF), and the reader might get a bit confused in my opinion:
If zarr contains world coordinates, can we not directly go from world to image? Why do we need ECEF, which data uses ECEF?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ECEF is a "safe" space, in the sense that everything can be converted into ECEF (semantic, satellite, world). Both semantic and satellite have matrices that go from their space into ECEF, but not to world (we build them at runtime)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to clarify it a bit more

coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
Copy link
Contributor

@oliver-scheel oliver-scheel left a comment

Choose a reason for hiding this comment

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

I'd restructure the satellite section a bit, and address the TODOs (naming: world vs pose, then origin in PAO - less important).

coords_systems.md Outdated Show resolved Hide resolved
coords_systems.md Outdated Show resolved Hide resolved
Copy link
Contributor

@melights melights left a comment

Choose a reason for hiding this comment

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

@lucabergamini @oliver-scheel I added the origin of the world coordinate system with google map link.

Copy link
Contributor

@oliver-scheel oliver-scheel left a comment

Choose a reason for hiding this comment

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

Nice, LGTM :)

@lucabergamini
Copy link
Contributor Author

Great, I'll merge this now. As we're changing the dataset structure on the disk something will change also here, but I'll start a PR later today (hopefully) to include the change-log. Thanks everyone!

@lucabergamini lucabergamini merged commit f7c5110 into master Aug 21, 2020
@yawei-ye yawei-ye deleted the lberg/doc-ref-systems branch August 24, 2020 16:49
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.

7 participants