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

Inconsistent conventions in function names #2781

Open
ducha-aiki opened this issue Feb 5, 2024 · 8 comments
Open

Inconsistent conventions in function names #2781

ducha-aiki opened this issue Feb 5, 2024 · 8 comments
Labels
help wanted Extra attention is needed

Comments

@ducha-aiki
Copy link
Member

Describe the bug

Here the points go second
https://kornia.readthedocs.io/en/latest/geometry.linalg.html#kornia.geometry.linalg.transform_points

Here first
https://kornia.readthedocs.io/en/latest/geometry.camera.perspective.html

Reproduction steps

https://twitter.com/Parskatt/status/1754501114777456916

Expected behavior

Not be confusing.

Environment

N/a

Additional context

No response

@ducha-aiki ducha-aiki added the help wanted Extra attention is needed label Feb 5, 2024
@edgarriba
Copy link
Member

what is the confusion here ? even though you can use one to solve the other, in principle the functions in principle are unrelated.

The first, applies a generic transformation to coordinates system of a set points given the mapping transformation

points_in_robot = transform_points(robot_from_world, points_in_world) 

the second has a specific purpose to project a set of 3d points to the z1 image plane given the calibration parameters of an affine model. The last is an old function that's is planned to be replaced by the new cameras api (based on sophus conventions): https://kornia.readthedocs.io/en/latest/geometry.camera.html

@ducha-aiki
Copy link
Member Author

Well, it would be really simple and great, if we use same idea - points first, or points last, across all the library.
Otherwise, one has to consistently look at the documentation.
Old or not, it doesn't matter.

@Parskatt
Copy link
Contributor

@Parskatt
Copy link
Contributor

Parskatt commented Feb 13, 2024

This inconsistency makes it confusing to use.
To me, projection is P: 3x4, intrinsics are K: 3x3, and pose is R: 3x3 ,t: 3x1 alt. T: 4x4

unproject_points should take K + image_coords + depth -> 3D points
transform_points should take 3D points + either (R, t) or T -> 3D points
projection_from_KRt takes K + R + t -> P
project_points should take 3D points + P -> 2D points

For me this is by far the most common usecase of these functions. Am I supposed to be creating PinholeCamera objects? To me the functional form is much easier to read.

I mean I can just write these functions myself instead, but I'd like to be able to use standardized stuff.

@edgarriba
Copy link
Member

@Parskatt thanks for the feedback.

The unproject_points / project_points were originally inspired by the tf.graphics.camera api and possibly the new way to use hem will be following the new Kornia apis here for Projections / Distortion.

The new apis are coming from Sophus / sophus-rs and a bit of Pangolin that have years of iterations for production in known companies and we use internally in our group for camera calibration and slam stuff. Possibly kornia-rs will adopt some of this too since the idea is to converge more and more to the Sophus api.

projection_from_KRt is part of the epipolar geometry package that's is actually a port from OpenCV SfM module which i contributed many years ago and actually coming from Libmv (backend mv library for Blender).

transform_points was kind the first kornia (torchgeometry at Arraiy times) function meant to be generic points coordinate system operator.

That's the context with all the why's for this apis -- said that, i'm totally happy that we define together a standard apif for the calibration / geometry api that makes sense for kornia context and whom we useit everyday. If you like we can open a design docs and discuss there the apis. Keep in mind backward compatibilities and such based on our new releases schedule.

/cc @cjpurackal since he's involved too with the new sensors and lie api

@ducha-aiki
Copy link
Member Author

@edgarriba exactly, we have inconsistent inspirations, and we should unify them. Users don't care about the history

@Parskatt
Copy link
Contributor

While I'm at it, why is this:
Screenshot_20240214_115137_Chrome.jpg

Composed on the left? I expected it would be on the right.

@edgarriba
Copy link
Member

Composed on the left? I expected it would be on the right.

that was an arbitrary choice i guess when that function was created. Depending on the application you would like in one or the other side, I'd be in favor in deprecating that and just teaching our users to use as follows:

these all should be equivalent (rigid body transform and points conventions are explained here)

t1_from_t2 = compose_transform(t1_from_t0, t0_from_t2)
t1_from_t2 = compose_transform(t0_from_t1.inverse(), t0_from_t2)
t1_from_t2 = compose_transform(t1_from_t0, t2_from_t0.inverse())
t1_from_t2 = compose_transform(t0_from_t1.inverse(), t2_from_t0.inverse())
t1_from_t2 = compose_transform(t2_from_t0, t1_from_t0.inverse()).inverse()

where compose_transform is just a convenient/tested function that handles matmul and batching if existing. That's my plan at least for kornia-rs which eventually will be the backend for small rank matrices. Pytorch is extremely inefficient at that link1 link2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants