Closed sbriseid closed 2 months ago
@sbriseid hey Sverre! thanks for raising this; quads have been an experimental feature (was not in the paper)
could you let me know if this change lines up with your expectation?
Not quite. That fixes the issue with the area, and was my first pick (except I didn't use a roll since I'm a tensor newbie). But then I realized that such a solution would mess up the calculation of the angles, which relies on the single shift. Hence I used an additional shift operation for the quad case, applied after calculating the angles.
I am very impressed by the work you and Marcus have done! The autoencoder is already very good, with the transformer improving on an almost daily basis it seems.
@sbriseid oh got it, should be good now :pray:
and yes, thankfully had Marcus stress testing it
this repository wouldn't have made it without all his feedback!
and of course, thank you for reviewing the computed quad features!
Now it should be correct!
I haven't tested the quad feature, this was after reading through the code to understand the data flow. Being a mathematician I am able to follow most of the mathematics in the code, unless it is hidden inside some wizardry tensor operations :)
@sbriseid math is where it is at
if only i knew what i would be doing today, i would have studied it more
Hi @lucidrains and @MarcusLoppe!
The calculation of the area for a quad in get_derived_face_features is missing a shift. The current code will calculate the area for the triangle defined by v0, v1 & v3, with v2 missing from edge1 & edge2.
By using a shift value of 2 for the quad case the edges will be the required diagonals v0 - v2 & v1 - v3. Since the single shift version is needed for the computation of the angles a second shift operation is needed:
I am here assuming that the vertices in the quad have a CCW ordering (or CW).