feat: Point coordinate regulators #118

Merged
Vectornaut merged 9 commits from glen/dyna3:pointCoordRegulators into main 2025-10-13 22:52:03 +00:00
Showing only changes of commit adc60ac5c1 - Show all commits

View file

@ -308,10 +308,11 @@ impl Element for Point {

Our current convention is that all the lines in a block, including blank lines, should have the block indentation. Under that convention, this blank line would need to be indented by four spaces, just like the lines above it, below it, and elsewhere in the impl block.

Our current convention is that all the lines in a block, including blank lines, should have the block indentation. Under that convention, this blank line would need to be indented by four spaces, just like the lines above it, below it, and elsewhere in the `impl` block.

A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line). I will go ahead and remove all trailing whitespace for consistency in an immediately following PR.

A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line). I will go ahead and remove all trailing whitespace for consistency in an immediately following PR.

A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line).

This is the convention the Rust style guide recommends, so I'm fine with it, even though it's not my favorite.

I have a slight preference for keeping this pull request consistent with the existing style, and removing all the trailing whitespace at once in the upcoming pull request.

> A much stronger convention, universal among all projects I have ever worked on, is no trailing whitespace (on any line). This is the convention the [Rust style guide](https://doc.rust-lang.org/beta/style-guide/index.html#trailing-whitespace) recommends, so I'm fine with it, even though it's not my favorite. I have a slight preference for keeping this pull request consistent with the existing style, and removing all the trailing whitespace at once in the upcoming pull request.
fn default_regulators(self: Rc<Self>) -> Vec<Rc<dyn Regulator>> {
all::<Axis>()
.map(|axis| {
Rc::new(PointCoordinateRegulator::new(self.clone(), axis))
as Rc::<dyn Regulator>
})
.map(
|axis| Rc::new(
PointCoordinateRegulator::new(self.clone(), axis)
) as Rc::<dyn Regulator>
)
.collect()
}
@ -538,19 +539,32 @@ impl PointCoordinateRegulator {
let measurement = subject.representation().map(
move |rep| rep[axis as usize]
);
let set_point = create_signal(SpecifiedValue::from_empty_spec());
Self { subject, axis, measurement, set_point, serial: Self::next_serial() }
let serial = Self::next_serial();
Self { subject, axis, measurement, set_point, serial }
}
}
impl Serial for PointCoordinateRegulator {
fn serial(&self) -> u64 { self.serial }
fn serial(&self) -> u64 {
self.serial
}
}
impl Regulator for PointCoordinateRegulator {
fn subjects(&self) -> Vec<Rc<dyn Element>> { vec![self.subject.clone()] }
fn measurement(&self) -> ReadSignal<f64> { self.measurement }
fn set_point(&self) -> Signal<SpecifiedValue> { self.set_point }
fn subjects(&self) -> Vec<Rc<dyn Element>> {
Vectornaut marked this conversation as resolved Outdated

Design

This kind of interaction between problem posers is something we didn't anticipate when designing the ProblemPoser trait. The current implementation seems to work fine, but it feels awkward: to me, it's clearly twisting the problem poser system into doing something it's not designed for. I'd be okay with merging it for now with a /* KLUDGE */ flag and filing a corresponding maintenance issue, especially if the best way to resolve the tension would be to redesign the ProblemPoser trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request.

Testing

To test the interaction system, I propose adding diagnostic logs that show the frozen variables. We could do throw this in by replacing the log call

// log the Gram matrix
console_log!("Gram matrix:\n{}", problem.gram);

with

// log the Gram matrix and frozen entries
console_log!("Gram matrix:\n{}", problem.gram);
console_log!("Frozen entries:\n{}", problem.frozen);

I tried this out locally, and it confirms that the interaction system is working in at least some cases.

Commenting

I'd suggest saying "norm component" instead of "coradius" to keep the comment consistent with the named constants. Here's a possible rewrite, which also incorporates the /* KLUDGE */ flag suggested above and uses the capitalization convention that most other comments follow.

// if the three representation components that specify the
// point's Euclidean coordinates have been frozen, then freeze
// the norm component too
/* KLUDGE */
// we didn't anticipate this kind of interaction when we
// designed the problem poser system, so the implementation
// feels awkward
### Design This kind of interaction between problem posers is something we didn't anticipate when designing the `ProblemPoser` trait. The current implementation seems to work fine, but it feels awkward: to me, it's clearly twisting the problem poser system into doing something it's not designed for. I'd be okay with merging it for now with a `/* KLUDGE */` flag and filing a corresponding maintenance issue, especially if the best way to resolve the tension would be to redesign the `ProblemPoser` trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request. ### Testing To test the interaction system, I propose adding diagnostic logs that show the frozen variables. We could do throw this in by replacing the log call ```rust // log the Gram matrix console_log!("Gram matrix:\n{}", problem.gram); ``` with ```rust // log the Gram matrix and frozen entries console_log!("Gram matrix:\n{}", problem.gram); console_log!("Frozen entries:\n{}", problem.frozen); ``` I tried this out locally, and it confirms that the interaction system is working in at least some cases. ### Commenting I'd suggest saying "norm component" instead of "coradius" to keep the comment consistent with the named constants. Here's a possible rewrite, which also incorporates the `/* KLUDGE */` flag suggested above and uses the capitalization convention that most other comments follow. ```rust // if the three representation components that specify the // point's Euclidean coordinates have been frozen, then freeze // the norm component too /* KLUDGE */ // we didn't anticipate this kind of interaction when we // designed the problem poser system, so the implementation // feels awkward ```

@Vectornaut wrote in #118 (comment):

the best way to resolve the tension would be to redesign the ProblemPoser trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request.

What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.

@Vectornaut wrote in https://code.studioinfinity.org/StudioInfinity/dyna3/pulls/118#issuecomment-3259: > the best way to resolve the tension would be to redesign the `ProblemPoser` trait. If you can think of a more local way to resolve the tension, though, it would be great to get that into this pull request. What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.

OK, actually, I am fine with leaving this "awkward" implementation for now, because it is covered by issue #90, where I have added a comment explicitly highlighting it. And similarly, I don't think any highlighting comment is needed, because we have that information in issue #90; we only want any piece of information in one place.

OK, actually, I am fine with leaving this "awkward" implementation for now, because it is covered by issue #90, where I have added a comment explicitly highlighting it. And similarly, I don't think any highlighting comment is needed, because we have that information in issue #90; we only want any piece of information in one place.

I agree that the comment in issue #90 is enough to remind us about this.

What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks.

We discussed this a little during our check-in meeting, and I jotted down a comment about our conclusions on issue #90.

I agree that the comment in issue #90 is enough to remind us about this. > What sort of a redesign would you have in mind? How would you add this extra frozen coordinate? Would you want the pose method of a Point to check if it had three frozen spatial coordinates, and if so freeze the norm component as well? Please provide guidance as to how this "ought" to be done, and let's just go ahead and do it the "right" way. Thanks. We discussed this a little during our check-in meeting, and I jotted down a [comment](https://code.studioinfinity.org/StudioInfinity/dyna3/issues/90#issuecomment-3288) about our conclusions on issue #90.
vec![self.subject.clone()]
}
fn measurement(&self) -> ReadSignal<f64> {

For consistency with InversiveDistanceRegulator, I'd change the panic message to:

Subject should be indexed before point coordinate regulator writes problem data

For consistency with `InversiveDistanceRegulator`, I'd change the panic message to: > Subject should be indexed before point coordinate regulator writes problem data

As far as I can see from the code, "must" is correct. I will change both messages to "Subject must be indexed before a regulator writes problem data".

As far as I can see from the code, "must" is correct. I will change both messages to "Subject must be indexed before a regulator writes problem data".

Actually, there were a bunch of such messages, and if the common format is actually important, repeating that wording over and over was not DRY. So I implemented a little message helper and used it consistently. This changed some existing error messages a bit, but they are definitely now all uniform (and some are slightly more informative, i.e., you get the id of an unindexed subject of a regulator now).

Actually, there were a bunch of such messages, and if the common format is actually important, repeating that wording over and over was not DRY. So I implemented a little message helper and used it consistently. This changed some existing error messages a bit, but they are definitely now all uniform (and some are slightly more informative, i.e., you get the id of an unindexed subject of a regulator now).

The helper is useful, and I like the more informative error messages! To help make it clear how the helper is supposed to be used, I might make some or all of the following changes.

  • Add more detail to the helper name: maybe something like index_required_message, missing_index_panic_message, or index_before_posing_message?
  • Rename the thing argument to something like role or kind_of_thing. Or, more broadly, name the arguments something like object_kind, object_name, actor?

As far as I can see from the code, "must" is correct.

Agreed.

The helper is useful, and I like the more informative error messages! To help make it clear how the helper is supposed to be used, I might make some or all of the following changes. - Add more detail to the helper name: maybe something like `index_required_message`, `missing_index_panic_message`, or `index_before_posing_message`? - Rename the `thing` argument to something like `role` or `kind_of_thing`. Or, more broadly, name the arguments something like `object_kind`, `object_name`, `actor`? > As far as I can see from the code, "must" is correct. Agreed.

To me, the placement of the new helper is kind of weird to me. It's right in the middle of the Sphere definition and implementation section, which makes it seem associated with Sphere, when in fact it's relevant to many problem-posers. It might fit better above the definition of the ProblemPoser trait, although I'd be open to other ideas about where to put it.

To me, the placement of the new helper is kind of weird to me. It's right in the middle of the `Sphere` definition and implementation section, which makes it seem associated with `Sphere`, when in fact it's relevant to many problem-posers. It might fit better above the definition of the `ProblemPoser` trait, although I'd be open to other ideas about where to put it.

Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. So not quite sure how to make the parameter name more specific without making it a bit misleading. Will ponder.

Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. So not quite sure how to make the parameter name more specific without making it a bit misleading. Will ponder.

Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it.

Great!

Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject.

The main thing I'd like to clarify, if possible, is that thing and name go together, and that thing is a type or role, while name is an identifier.

> Ok will move message helper up a bit and comment it. As far as the name, I would like to avoid undue verbosity for a private local helper, I will see if I can sharpen it a bit without lengthening it. Great! > Finally the first argument became "thing" because there actually isn't any consistent category: for elements it ends up being the element whereas for regulators it's a subject. The main thing I'd like to clarify, if possible, is that `thing` and `name` go together, and that `thing` is a type or role, while `name` is an identifier.
self.measurement
}
Vectornaut marked this conversation as resolved Outdated

I've been wrapping comments at 80 characters throughout the code. Under that convention, this comment would need to be reflowed.

I prefer to use comments only at the beginnings of a code sections (which are separated by blank lines). To me, the stuff after this comment does feel like a distinct section, so I'd add a blank line before the comment.

I think the comment could be made a little clearer and more terse, although I won't insist on it. Here's a wording I like.

// if all three of the subject's spatial coordinates have been
// frozen, then freeze its norm component too

(I've also been writing comments in lower case except for stuff like proper nouns and acronyms, but that seems more controversial, so I won't fuss about it in this pull request.)

I've been wrapping comments at 80 characters throughout the code. Under that convention, this comment would need to be reflowed. I prefer to use comments only at the beginnings of a code sections (which are separated by blank lines). To me, the stuff after this comment does feel like a distinct section, so I'd add a blank line before the comment. I think the comment could be made a little clearer and more terse, although I won't insist on it. Here's a wording I like. ``` // if all three of the subject's spatial coordinates have been // frozen, then freeze its norm component too ``` (I've also been writing comments in lower case except for stuff like proper nouns and acronyms, but that seems more controversial, so I won't fuss about it in this pull request.)

I will adhere to the 80-character line limit (which I am a strong supporter of), which I inadvertently violated. I will reword the comment as suggested. I feel the comment breaks up the code sufficiently, meaning no need for a blank line.

I will adhere to the 80-character line limit (which I am a strong supporter of), which I inadvertently violated. I will reword the comment as suggested. I feel the comment breaks up the code sufficiently, meaning no need for a blank line.

Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.

Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.

Thanks for reflowing and rewording the comment!

Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them.

I'd rather do this in a separate pull request. Reflowing code that this pull request adds or rewrites is okay with me, but I have a slight preference for also keeping that code consistent with the existing style.

The other reflows in commit c0e6ebf affect code lines, rather than comment lines. I've been less strict about wrapping code to 80 characters, because sometimes I think the value of keeping a line together outweighs the cost of going slightly over 80 characters. I'd be fine with switching to a more consistent convention, like the line width and comment width rules from the Rust style guide, but I think we should discuss what rule we want to adopt and then implement it in its own pull request.

Thanks for reflowing and rewording the comment! > Actually there are many 80-character line violations; again, I didn't realize. I will fix the ones in this source file, and we can fix other source files as we edit them, I guess, or do a PR for them. I'd rather do this in a separate pull request. Reflowing code that this pull request adds or rewrites is okay with me, but I have a slight preference for also keeping that code consistent with the existing style. The other reflows in commit c0e6ebf affect code lines, rather than comment lines. I've been less strict about wrapping code to 80 characters, because sometimes I think the value of keeping a line together outweighs the cost of going slightly over 80 characters. I'd be fine with switching to a more consistent convention, like the [line width](https://doc.rust-lang.org/beta/style-guide/index.html#indentation-and-line-width) and [comment width](https://doc.rust-lang.org/beta/style-guide/index.html#comments) rules from the Rust style guide, but I think we should discuss what rule we want to adopt and then implement it in its own pull request.

I am super confused. I cannot understand a different line length limit for comments and code; comments are code. (The point of code is to communicate what a program does, bith to humans and compilers. Comments are an integral part of that.) Also the point of line length limits is to manage the horizontal width of the code display (so the reason I like 80 is that my display devices are all right around 160-170 characters wide at comfortable font sizes, and it is at times very useful to display two source files side by side). Again it doesn't make sense to have different limits for code and comments. I only messed with this because you pointed out a violation of our 80-character limit. So naturally when I saw there were many others, I fixed them. I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?

I am super confused. I cannot understand a different line length limit for comments and code; comments are code. (The point of code is to communicate what a program does, bith to humans and compilers. Comments are an integral part of that.) Also the point of line length limits is to manage the horizontal width of the code display (so the reason I like 80 is that my display devices are all right around 160-170 characters wide at comfortable font sizes, and it is at times very useful to display two source files side by side). Again it doesn't make sense to have different limits for code and comments. I only messed with this because you pointed out a violation of our 80-character limit. So naturally when I saw there were many others, I fixed them. I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?

I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok?

Making a later pull request to impose a consistent line width limit for both comments and code is okay with me. Maybe you can decide what limit you prefer while preparing that pull request? From my experience so far, I can see why the style guide authors chose 100 characters: wrapping strictly at 80 characters expands the code vertically quite a bit.

> I can revert all these changes, and do a PR after the whitespace one fixing all the 80-character violations in the whole code base. Ok? Making a later pull request to impose a consistent line width limit for both comments and code is okay with me. Maybe you can decide what limit you prefer while preparing that pull request? From my experience so far, I can see why the style guide authors chose 100 characters: wrapping strictly at 80 characters expands the code vertically quite a bit.

The point of line length convention is to keep the code fitting horizontally in some amount of space. For me the salient amount of space is "narrow enough to fit two full lines side-by-side" which happens to work out to the old-school-standard 80 characters, more or less by coincidence. So I'd like to just stick with 80, thanks.

As for the resulting vertical expansion, well, right now we have a lot of line noise expanding things vertically. A very large fraction of our lines contain just 0-3 characters. I am looking forward to cleaning that up with Husht. I also find that if one is not too rigid about where one must or must not break lines, vertical expansion isn't too bad. In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment:

  • switch to a 3-character standard indent. I personally find this the best compromise between visual salience, and not forcing nested code too far to the right so that it needs to break a lot.
  • switch to putting all closing punctuation at the end of the line that it closes, rather than skipping a line for it. After all (and this is one of the main motivations for Husht), all that closing punctuation is redundant with the indenting. E.g.
fn myfn(arg: Type) {
   if condition {
      take_action(arg, other, long,
         arguments, in, list)}}

fn my otherfn() -> ReturnType {
   ...
The point of line length convention is to keep the code fitting horizontally in some amount of space. For me the salient amount of space is "narrow enough to fit two full lines side-by-side" which happens to work out to the old-school-standard 80 characters, more or less by coincidence. So I'd like to just stick with 80, thanks. As for the resulting vertical expansion, well, right now we have a lot of line noise expanding things vertically. A very large fraction of our lines contain just 0-3 characters. I am looking forward to cleaning that up with Husht. I also find that if one is not too rigid about where one must or must not break lines, vertical expansion isn't too bad. In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment: * switch to a 3-character standard indent. I personally find this the best compromise between visual salience, and not forcing nested code too far to the right so that it needs to break a lot. * switch to putting all closing punctuation at the end of the line that it closes, rather than skipping a line for it. After all (and this is one of the main motivations for Husht), all that closing punctuation is redundant with the indenting. E.g. ``` fn myfn(arg: Type) { if condition { take_action(arg, other, long, arguments, in, list)}} fn my otherfn() -> ReturnType { ... ```

So I'd like to just stick with 80, thanks.

Sounds good!

In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment

Each of those changes would bother me more than seeing more vertical expansion, so I'd rather hold off for now.

> So I'd like to just stick with 80, thanks. Sounds good! > In the meantime, I would be perfectly happy to do either or both of the following two things, but they might each be a bridge too far for you at the moment Each of those changes would bother me more than seeing more vertical expansion, so I'd rather hold off for now.
fn set_point(&self) -> Signal<SpecifiedValue> {
self.set_point
}
}
impl ProblemPoser for PointCoordinateRegulator {
@ -558,22 +572,25 @@ impl ProblemPoser for PointCoordinateRegulator {
self.set_point.with_untracked(|set_pt| {
if let Some(val) = set_pt.value {
let col = self.subject.column_index().expect(
Vectornaut marked this conversation as resolved

Flagging the inconsistent bracket formatting for later. Right now, this would be formatted as { index, value } elsewhere, but during review we discussed potentially changing this convention in the future.

Flagging the inconsistent bracket formatting for later. Right now, this would be formatted as `{ index, value }` elsewhere, but during review we discussed potentially changing this convention in the future.

It might shock you to know that I don't actually particularly care whether the braces-on-one-line spacing is uniform. If we both agree not to change them unless we touch a line, we could each just write this one the way we're comfortable writing. It's not like either way is particularly hard to read... Or we could decide to be uniform. It's all cool.

It might shock you to know that I don't actually particularly care whether the braces-on-one-line spacing is uniform. If we both agree not to change them unless we touch a line, we could each just write this one the way we're comfortable writing. It's not like either way is particularly hard to read... Or we could decide to be uniform. It's all cool.
"Subject must be indexed before point-coordinate regulator poses.");
"Subject should be indexed before point coordinate regulator writes problem data"
);
problem.frozen.push(self.axis as usize, col, val);
// Check if all three spatial coordinates have been frozen, and if so,
// freeze the norm component as well
let mut coords = [0.0; Axis::CARDINALITY];
let mut nset: usize = 0;
// if all three of the subject's spatial coordinates have been
// frozen, then freeze its norm component too
let mut coords_frozen = [0.0; Axis::CARDINALITY];
let mut n_set: usize = 0;
for &MatrixEntry { index, value } in &(problem.frozen) {
if index.1 == col && index.0 < Axis::CARDINALITY {
nset += 1;
coords[index.0] = value
let (row_frozen, col_frozen) = index;
if col_frozen == col && row_frozen < Axis::CARDINALITY {
n_set += 1;
coords_frozen[row_frozen] = value
}
}
if nset == Axis::CARDINALITY {
let [x, y, z] = coords;
problem.frozen.push(
Point::NORM_COMPONENT, col, point(x,y,z)[Point::NORM_COMPONENT]);
if n_set == Axis::CARDINALITY {
let [x, y, z] = coords_frozen;
let norm = point(x, y, z)[Point::NORM_COMPONENT];
problem.frozen.push(Point::NORM_COMPONENT, col, norm);
}
}
});