Dispatch normalization routines correctly #87

Merged
glen merged 4 commits from Vectornaut/dyna3:dispatch-normalization into main 2025-06-04 21:01:14 +00:00
2 changed files with 14 additions and 13 deletions
Showing only changes of commit f332f755e0 - Show all commits

View file

@ -18,9 +18,9 @@ use crate::{
Q,
change_half_curvature,
local_unif_to_std,
normalize_mut_point,
normalize_mut_sphere,
point,
project_point_to_normalized,
project_sphere_to_normalized,
realize_gram,
sphere,
ConfigSubspace,
@ -108,8 +108,9 @@ pub trait Element: Serial + ProblemPoser + DisplayItem {
// element is responsible for keeping this set up to date
fn regulators(&self) -> Signal<BTreeSet<Rc<dyn Regulator>>>;
glen marked this conversation as resolved Outdated

Do you like the word "normalize" or "canonicalize"? I feel like the latter more explicitly means "choose the favored representative of an equivalence class" (which I think is mainly what's going on here) and the former can have broader meanings in which the entity is actually changed, like when you find a unit vector in the same direction -- I think that's sometimes called normalization. Only make a change if you feel like "canonicalize" would be a better/clearer choice.

Do you like the word "normalize" or "canonicalize"? I feel like the latter more explicitly means "choose the favored representative of an equivalence class" (which I think is mainly what's going on here) and the former can have broader meanings in which the entity is actually changed, like when you find a unit vector in the same direction -- I think that's sometimes called normalization. Only make a change if you feel like "canonicalize" would be a better/clearer choice.

In my mind, an element is represented by a line through the origin in \mathbb{R}^{4,1}, so choosing a favored representative is equivalent to choosing a particular scaling. That's why I used the word "normalization." However, the implementation of normalize_rep_mut for spheres actually changes the equivalence class. That's because the only thing we use normalize_rep_mut for right now is to keep elements normalized during nudging, and normalizing in a way that preserves the equivalence class makes nudging less stable. If both "normalize" and "canonicalize" imply staying within an equivalence class for you, then maybe we need another term entirely.

In my mind, an element is represented by a line through the origin in $\mathbb{R}^{4,1}$, so choosing a favored representative is equivalent to choosing a particular scaling. That's why I used the word "normalization." However, the implementation of `normalize_rep_mut` for spheres actually changes the equivalence class. That's because the only thing we use `normalize_rep_mut` for right now is to keep elements normalized during nudging, and normalizing in a way that preserves the equivalence class makes nudging less stable. If both "normalize" and "canonicalize" imply staying within an equivalence class for you, then maybe we need another term entirely.

Ah, hmm, I hadn't appreciated that the result of normalize_rep_mut might be in a different equivalence class. "Normalize" does not by itself connote "stay in the same equivalence class, but "normalize representation" does seem to imply strongly that we are just choosing a different representation of the same entity. So I would agree the name needs to change -- but just "normalize" without the "representation" would be OK, since we're not just picking a different representation of the same geometric entity. If you don't like that, perhaps "stabilize" or "standardize" or "choose_canonical" or "nearby_canonical" or something?

Ah, hmm, I hadn't appreciated that the result of normalize_rep_mut might be in a different equivalence class. "Normalize" does not by itself connote "stay in the same equivalence class, but "normalize representation" does seem to imply strongly that we are just choosing a different representation of the same entity. So I would agree the name needs to change -- but just "normalize" without the "representation" would be OK, since we're not just picking a different representation of the same geometric entity. If you don't like that, perhaps "stabilize" or "standardize" or "choose_canonical" or "nearby_canonical" or something?

How about something like proj_to_normalized, to communicate that we're projecting the given representation vector to the normalized configuration variety for the given element type?

So I would agree the name needs to change -- but just "normalize" without the "representation" would be OK, since we're not just picking a different representation of the same geometric entity.

The rep isn't working as intended in the name normalize_rep_mut anyway. I added it to emphasize that this method acts on the representation vector provided as an argument, not the vector representing the element the method is called from. This makes it different from the normalize_mut methods for nalgebra matrices.

(Using an associated function instead of a method would be the best way to clarify this, but I don't think it's possible to call associated functions from trait objects. This complaint about the limitation and this unsuccessful attempt to lift the limitation seem to demonstrate that it exists, or at least used to exist. If you want, though, I can try again to use an associated function and look at exactly what compiler error it produced.)

How about something like `proj_to_normalized`, to communicate that we're projecting the given representation vector to the normalized configuration variety for the given element type? > So I would agree the name needs to change -- but just "normalize" without the "representation" would be OK, since we're not just picking a different representation of the same geometric entity. The `rep` isn't working as intended in the name `normalize_rep_mut` anyway. I added it to emphasize that this method acts on the representation vector provided as an argument, not the vector representing the element the method is called from. This makes it different from the [`normalize_mut`](https://docs.rs/nalgebra/latest/nalgebra/base/type.DMatrix.html#method.normalize_mut) methods for nalgebra matrices. (Using an associated function instead of a method would be the best way to clarify this, but I don't think it's possible to call associated functions from trait objects. [This complaint](https://users.rust-lang.org/t/trait-object-safety-still-confuses-me/39699/4) about the limitation and [this unsuccessful attempt](https://github.com/rust-lang/rfcs/pull/2886) to lift the limitation seem to demonstrate that it exists, or at least used to exist. If you want, though, I can try again to use an associated function and look at exactly what compiler error it produced.)

Sure, any of project_to_canonical or project_to_normal or project_to_normalized or canonical_projection or normal_projection would be fine, whatever seems simplest/clearest to you. I'd suggest that with a name that's already this long, there's no point in abbreviating with proj -- just use whatever full word proj would be an abbreviation for. Using a method is fine, don't worry about the associated function thing.

Sure, any of `project_to_canonical` or `project_to_normal` or `project_to_normalized` or `canonical_projection` or `normal_projection` would be fine, whatever seems simplest/clearest to you. I'd suggest that with a name that's already this long, there's no point in abbreviating with `proj` -- just use whatever full word `proj` would be an abbreviation for. Using a method is fine, don't worry about the associated function thing.

I just pushed a commit with better names (54b34e0582). My look-over before pushing was a bit cursory; I'll look at it harder later today.

I just pushed a commit with better names (`54b34e0582`). My look-over before pushing was a bit cursory; I'll look at it harder later today.

I've looked at the renaming commit again and I still think it's fine, so let me know what you think!

I've looked at the renaming commit again and I still think it's fine, so let me know what you think!
// normalize a representation vector for this kind of element
fn normalize_mut_rep(&self, rep: &mut DVector<f64>);
// project a representation vector for this kind of element onto its
// normalization variety
fn project_to_normalized(&self, rep: &mut DVector<f64>);
// the configuration matrix column index that was assigned to the element
// last time the assembly was realized, or `None` if the element has never
@ -225,8 +226,8 @@ impl Element for Sphere {
self.regulators
}
fn normalize_mut_rep(&self, rep: &mut DVector<f64>) {
normalize_mut_sphere(rep);
fn project_to_normalized(&self, rep: &mut DVector<f64>) {
project_sphere_to_normalized(rep);
}
fn column_index(&self) -> Option<usize> {
@ -321,8 +322,8 @@ impl Element for Point {
self.regulators
}
fn normalize_mut_rep(&self, rep: &mut DVector<f64>) {
normalize_mut_point(rep);
fn project_to_normalized(&self, rep: &mut DVector<f64>) {
project_point_to_normalized(rep);
}
fn column_index(&self) -> Option<usize> {
@ -789,7 +790,7 @@ impl Assembly {
// step the element along the deformation and then
// restore its normalization
*rep += motion_proj.column(column_index);
elt.normalize_mut_rep(rep);
elt.project_to_normalized(rep);
},
None => {
console_log!("No velocity to unpack for fresh element \"{}\"", elt.id())

View file

@ -35,9 +35,9 @@ pub fn sphere_with_offset(dir_x: f64, dir_y: f64, dir_z: f64, off: f64, curv: f6
])
}
// normalize a sphere's representation vector by contracting toward the last
// coordinate axis
pub fn normalize_mut_sphere(rep: &mut DVector<f64>) {
// project a sphere's representation vector to the normalization variety by
// contracting toward the last coordinate axis
pub fn project_sphere_to_normalized(rep: &mut DVector<f64>) {
let q_sp = rep.fixed_rows::<3>(0).norm_squared();
let half_q_lt = -2.0 * rep[3] * rep[4];
let half_q_lt_sq = half_q_lt * half_q_lt;
@ -46,7 +46,7 @@ pub fn normalize_mut_sphere(rep: &mut DVector<f64>) {
}
// normalize a point's representation vector by scaling
pub fn normalize_mut_point(rep: &mut DVector<f64>) {
pub fn project_point_to_normalized(rep: &mut DVector<f64>) {
rep.scale_mut(0.5 / rep[3]);
}