glen/dyna3
1
0
Fork 0
Code Issues 11 Pull Requests 1 Actions Projects Releases 2 Wiki Activity

Integrate engine into application prototype #15

Merged
glen merged 24 commits from engine-integration into main 2024-11-12 00:46:16 +00:00
Conversation 15 Commits 24 Files Changed 14 +947 -93
Vectornaut commented 2024-10-30 20:01:47 +00:00
Collaborator
Copy Link

Port the engine prototype to Rust, integrate it into the application prototype, and use it to enforce the constraints.

Features

To see the engine in action:

  1. Add a constraint by shift-clicking to select two spheres in the outline view and then hitting the 🔗 button
  2. Click a summary arrow to see the outline item for the new constraint
  3. Set the constraint's Lorentz product by entering a value in the text field at the right end of the outline item
    • The display should update as soon as you press Enter or focus away from the text field

The checkbox at the left end of a constraint outline item controls whether the constraint is active. Activating a constraint triggers a solution update. (Deactivating a constraint doesn't, since the remaining active constraints are still satisfied.)

Precision

The Julia prototype of the engine uses a generic scalar type, so you can pass in any type the linear algebra functions are implemented for. The examples use the adjustable-precision BigFloat type.

In the Rust port of the engine, the scalar type is currently fixed at f64. Switching to generic scalars shouldn't be too hard, but I haven't looked into which other types the linear algebra functions are implemented for.

Testing

To confirm quantitatively that the Rust port of the engine is working, you can go to the app-proto folder and:

  • Run some automated tests by calling cargo test.

  • Inspect the optimization process in a few examples calling the run-examples script. The first example that prints is the same as the Irisawa hexlet example from the engine prototype. If you go into engine-proto/gram-test, launch Julia, and then

    include("irisawa-hexlet.jl")
for (step, scaled_loss) in enumerate(history_alt.scaled_loss)
  println(rpad(step-1, 4), " | ", scaled_loss)
end

    you should see that it prints basically the same loss history until the last few steps, when the lower default precision of the Rust engine really starts to show.

A small engine revision

The Rust port of the engine improves on the Julia prototype in one part of the constraint-solving routine: projecting the Hessian onto the subspace where the frozen entries stay constant. The Julia prototype does this by removing the rows and columns of the Hessian that correspond to the frozen entries, finding the Newton step from the resulting "compressed" Hessian, and then adding zero entries to the Newton step in the appropriate places. The Rust port instead replaces each frozen row and column with its corresponding standard unit vector, avoiding the finicky compressing and decompressing steps.

To confirm that this version of the constraint-solving routine works the same as the original, I implemented it in Julia as realize_gram_alt_proj. The solutions we get from this routine match the ones we get from the original realize_gram to very high precision, and in the simplest examples (sphere-in-tetrahedron.jl and tetrahedron-radius-ratio.jl), the descent paths also match to very high precision. In a more complicated example (irisawa-hexlet.jl), the descent paths diverge about a quarter of the way into the search, even though they end up in the same place.

Port the engine prototype to Rust, integrate it into the application prototype, and use it to enforce the constraints. ### Features To see the engine in action: 1. Add a constraint by shift-clicking to select two spheres in the outline view and then hitting the 🔗 button 2. Click a summary arrow to see the outline item for the new constraint 2. Set the constraint's Lorentz product by entering a value in the text field at the right end of the outline item * *The display should update as soon as you press* Enter *or focus away from the text field* The checkbox at the left end of a constraint outline item controls whether the constraint is active. Activating a constraint triggers a solution update. (Deactivating a constraint doesn't, since the remaining active constraints are still satisfied.) ### Precision The Julia prototype of the engine uses a generic scalar type, so you can pass in any type the linear algebra functions are implemented for. The examples use the [adjustable-precision](https://docs.julialang.org/en/v1/base/numbers/#Base.MPFR.setprecision) `BigFloat` type. In the Rust port of the engine, the scalar type is currently fixed at `f64`. Switching to generic scalars shouldn't be too hard, but I haven't looked into [which other types](https://www.nalgebra.org/docs/user_guide/generic_programming) the linear algebra functions are implemented for. ### Testing To confirm quantitatively that the Rust port of the engine is working, you can go to the `app-proto` folder and: * Run some automated tests by calling `cargo test`. * Inspect the optimization process in a few examples calling the `run-examples` script. The first example that prints is the same as the Irisawa hexlet example from the engine prototype. If you go into `engine-proto/gram-test`, launch Julia, and then ``` include("irisawa-hexlet.jl") for (step, scaled_loss) in enumerate(history_alt.scaled_loss) println(rpad(step-1, 4), " | ", scaled_loss) end ``` you should see that it prints basically the same loss history until the last few steps, when the lower default precision of the Rust engine really starts to show. ### A small engine revision The Rust port of the engine improves on the Julia prototype in one part of the constraint-solving routine: projecting the Hessian onto the subspace where the frozen entries stay constant. The Julia prototype does this by removing the rows and columns of the Hessian that correspond to the frozen entries, finding the Newton step from the resulting "compressed" Hessian, and then adding zero entries to the Newton step in the appropriate places. The Rust port instead replaces each frozen row and column with its corresponding standard unit vector, avoiding the finicky compressing and decompressing steps. To confirm that this version of the constraint-solving routine works the same as the original, I implemented it in Julia as `realize_gram_alt_proj`. The solutions we get from this routine match the ones we get from the original `realize_gram` to very high precision, and in the simplest examples (`sphere-in-tetrahedron.jl` and `tetrahedron-radius-ratio.jl`), the descent paths also match to very high precision. In a more complicated example (`irisawa-hexlet.jl`), the descent paths diverge about a quarter of the way into the search, even though they end up in the same place.
Vectornaut added 13 commits 2024-10-30 20:01:48 +00:00 
Validate with the process inspection example tests, which print out
their results and optimization histories when run one at a time in
`--nocapture` mode.
In the process, notice that the tolerance scale adjustment was ported
wrong, and correct it.
Sycamore probably has a better way to do this, but this way works for
now.
Setting `bind:value` or `bind:valueAsNumber` for a number input seems to
restrict what you can type in it. We work around this by switching to
text inputs for now. We should probably switch back to number inputs if
we can, though, because they let us take advantage of the browser's
parsing and validation.
glen commented 2024-11-09 23:26:18 +00:00
Owner
Copy Link
  • Inspect the optimization process in a few examples calling the run-examples script. The first example that prints is the same as the Irisawa hexlet example from the engine prototype. If you go into engine-proto/gram-test, launch Julia, and then include("irisawa-hexlet.jl"), you should see that it prints basically the same loss history until the last few steps, when the lower default precision of the Rust engine really starts to show.

For me, the Julia file does not print a loss history, only the final loss, which is indeed far smaller than the loss achieved by the Rust version. But since this is ancillary to this PR, I am certainly not going to hold up merging it for this sake.

> * Inspect the optimization process in a few examples calling the `run-examples` script. The first example that prints is the same as the Irisawa hexlet example from the engine prototype. If you go into `engine-proto/gram-test`, launch Julia, and then `include("irisawa-hexlet.jl")`, you should see that it prints basically the same loss history until the last few steps, when the lower default precision of the Rust engine really starts to show. For me, the Julia file does not print a loss history, only the final loss, which is indeed far smaller than the loss achieved by the Rust version. But since this is ancillary to this PR, I am certainly not going to hold up merging it for this sake.
👍 1
glen commented 2024-11-09 23:29:37 +00:00
Owner
Copy Link
   Compiling sketch-outline v0.1.0 (/home/glen/code/dyna3/app-proto)
warning: function `point` is never used
 --> src/engine.rs:7:8
  |
7 | pub fn point(x: f64, y: f64, z: f64) -> DVector<f64> {
  |        ^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

warning: `sketch-outline` (bin "sketch-outline") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 7.10s

Please fix the warning and update this PR; we never want our code to be in a state in which it generates warnings. In fact, if there is a compiler flag to make warnings into errors, please turn it on. Thanks!

``` Compiling sketch-outline v0.1.0 (/home/glen/code/dyna3/app-proto) warning: function `point` is never used --> src/engine.rs:7:8 | 7 | pub fn point(x: f64, y: f64, z: f64) -> DVector<f64> { | ^^^^^ | = note: `#[warn(dead_code)]` on by default warning: `sketch-outline` (bin "sketch-outline") generated 1 warning Finished `dev` profile [unoptimized + debuginfo] target(s) in 7.10s ``` Please fix the warning and update this PR; we never want our code to be in a state in which it generates warnings. In fact, if there is a compiler flag to make warnings into errors, please turn it on. Thanks!
👍 1
glen commented 2024-11-09 23:50:11 +00:00
Owner
Copy Link

The Rust port instead replaces each frozen row and column with its corresponding standard unit vector, avoiding the finicky compressing and decompressing steps.

I know we discussed this in person, but the fact that the Julia solution paths diverged when you made this change was a little surprising and worrisome to me. What's the mathematical difference between the two approaches? Are they theoretically identical, with differences only occurring for numerical reasons? Is it the case that you don't get exact 0s in the resulting Newton step with the adjusted approach in Rust, only approximate 0s? If the latter is the case, and in fact exact 0s in the step are "better" because after all, that coordinate is clamped to a specific value and should not change at all, and if it's just programming inconvenience to go back to the original Julia method, then let's do so. I assume any time lost in doing the matrix reconfiguration is gained back in dealing with a Hessian with fewer rows/columns...

> The Rust port instead replaces each frozen row and column with its corresponding standard unit vector, avoiding the finicky compressing and decompressing steps. I know we discussed this in person, but the fact that the Julia solution paths diverged when you made this change was a little surprising and worrisome to me. What's the mathematical difference between the two approaches? Are they theoretically identical, with differences only occurring for numerical reasons? Is it the case that you don't get exact 0s in the resulting Newton step with the adjusted approach in Rust, only approximate 0s? If the latter is the case, and in fact exact 0s in the step are "better" because after all, that coordinate is clamped to a specific value and should not change at all, and if it's just programming inconvenience to go back to the original Julia method, then let's do so. I assume any time lost in doing the matrix reconfiguration is gained back in dealing with a Hessian with fewer rows/columns...
👍 1
glen requested changes 2024-11-10 00:52:01 +00:00
glen left a comment
Owner
Copy Link

OK, went through the code. Since this prototype seems like it has a chance to become the start of the actual code, I don't think it's too early to start trying to use some hygienic naming and organization, and that's what most of my comments are about. On the other hand, if your feeling is that essentially all of this will definitely get thrown away, I could be persuaded by an argument that it's not worth taking the time now. Let me know your thoughts.

OK, went through the code. Since this prototype seems like it has a chance to become the start of the actual code, I don't think it's too early to start trying to use some hygienic naming and organization, and that's what most of my comments are about. On the other hand, if your feeling is that essentially all of this will definitely get thrown away, I could be persuaded by an argument that it's not worth taking the time now. Let me know your thoughts.
👍 1
app-proto/src/assembly.rs Outdated
@ -10,13 +13,18 @@ pub struct Element {
pub label: String,
pub color: [f32; 3],
glen commented 2024-11-09 23:59:52 +00:00
Owner
Copy Link

Shouldn't we have at least a typedef somewhere for color representations, in case we use a different color space at some point? For example, we are almost certain to want to add an opacity level to our colors. (Certainly the final product will allow you to set the opacity of any element.)

Shouldn't we have at least a typedef somewhere for color representations, in case we use a different color space at some point? For example, we are almost certain to want to add an opacity level to our colors. (Certainly the final product will allow you to set the opacity of any element.)
👍 1
Vectornaut commented 2024-11-11 08:06:01 +00:00
Author
Collaborator
Copy Link

Done, with the type alias ElementColor.

Done, with the type alias `ElementColor`.
glen commented 2024-11-11 15:54:05 +00:00
Owner
Copy Link

OK, we will use that for now; the name implies we might have a different type for the color of something else, like the axes in a view (which aren't part of the Assembly). If we do start using this type for the colors of other things, we should rename it, I think.

OK, we will use that for now; the name implies we might have a different type for the color of something else, like the axes in a view (which aren't part of the Assembly). If we do start using this type for the colors of other things, we should rename it, I think.
glen marked this conversation as resolved
app-proto/src/assembly.rs Outdated
@ -10,13 +13,18 @@ pub struct Element {
pub label: String,
pub color: [f32; 3],
pub rep: DVector<f64>,
glen commented 2024-11-09 23:57:26 +00:00
Owner
Copy Link

Same comments go for rep here as in Constraint. Maybe it's "representative"? If your worry is that a full word will be too long, look for a shorter word, or pick a unique short acronym for this sort of thing in both places, and then clearly document what that acronym means and why you are using it. Maybe for now "document" just means "comment" until we have a decent doc system in place.

Same comments go for rep here as in Constraint. Maybe it's "representative"? If your worry is that a full word will be too long, look for a shorter word, or pick a unique short acronym for this sort of thing in both places, and then clearly document what that acronym means and why you are using it. Maybe for now "document" just means "comment" until we have a decent doc system in place.
Vectornaut commented 2024-11-11 03:29:35 +00:00
Author
Collaborator
Copy Link

Same comments go for rep here as in Constraint. Maybe it's "representative"?

It's for "representation"—a vector representation of the element, acted on by a linear representation of the 3d Möbius group. When I update the PR, I'll expand the property name to representation (in commit da008fd).

> Same comments go for rep here as in Constraint. Maybe it's "representative"? It's for "representation"—a vector representation of the element, acted on by a linear representation of the 3d Möbius group. When I update the PR, I'll expand the property name to `representation` (in commit da008fd).
glen commented 2024-11-11 19:57:27 +00:00
Owner
Copy Link

One other comment: some of the stuff in the assembly is intrinsic, i.e. there is a red sphere, constrained to have radius 1, containing a point with id A labeled A_{init}, say. Other stuff is contingent: the current realization of the red sphere has such-and-such representation, and point id A is selected. (I think the selection needs to be constant and shared across all views, so may as well be in the Assembly.) There may turn out to be practical reasons to separate those types of things in some way, but I certainly think there are conceptual advantages to doing so. For now, we may just want to keep in mind to choose names that suggest the intrinsic/contingent divide. It may be that "representation" is sufficiently suggestive in this way, or perhaps a name like "realization" would be better, and "currentRealization" is even stronger in this regard. Similarly, "selection" might already feel contingent enough; "currentlySelected" certainly would. This is not a request for any particular change to the current PR, just an item to keep in mind, although if you are moved to make any changes that's OK too.

One other comment: some of the stuff in the assembly is intrinsic, i.e. there _is_ a red sphere, constrained to have radius 1, containing a point with id A labeled `A_{init}`, say. Other stuff is contingent: the current realization of the red sphere has such-and-such representation, and point id A is selected. (I think the selection needs to be constant and shared across all views, so may as well be in the Assembly.) There may turn out to be practical reasons to separate those types of things in some way, but I certainly think there are conceptual advantages to doing so. For now, we may just want to keep in mind to choose names that suggest the intrinsic/contingent divide. It may be that "representation" is sufficiently suggestive in this way, or perhaps a name like "realization" would be better, and "currentRealization" is even stronger in this regard. Similarly, "selection" might already feel contingent enough; "currentlySelected" certainly would. This is not a request for any particular change to the current PR, just an item to keep in mind, although if you are moved to make any changes that's OK too.
glen marked this conversation as resolved
app-proto/src/assembly.rs Outdated
@ -14,0 +16,4 @@
pub constraints: BTreeSet<usize>,
// internal properties, not reflected in any view
pub index: usize
glen commented 2024-11-10 00:14:32 +00:00
Owner
Copy Link

I am confused; if this is internal, why is it pub? If it is important that this property never be reflected in a view (i.e., it is not really an intrinsic property of the element, just a contingent value for programming convenience) then there should be a way to enforce that views don't see it. Does Rust have anything like C++ friends or protected or something? Ideally, anything that shouldn't be accessible in a view would not be exposed in an interface to the assembly that views can get a hold of.

Also, I am guessing that this "index" is the same value that will appear for example in the pair in a Constraint that identifies the elements that are being constrained? If so, then the same typedef should be used here, and the names should be harmonized: if you land on ElementHandle, say, then this should be handle.

And then to soften my first paragraph: maybe we are encouraging views to use these handles for efficiency? Then we should not describe them as "internal" but instead document that they are not intrinsic properties of an element and not to be relied on across a reload of an assembly, but can be used in XXX circumstances and will be invariant over YYY operations, that sort of thing.

Oh, and then I see that Assembly has an elements_by_id property. Once you've settled on names for the relevant concept, say it's an ElementHandle, that should be renamed to something like handleOfId or elementHandleOfId.

Note I have been typing camelCase and your code is using snake_case. The former is more compact, and I am quite used to it. But I am not stuck on it. We should discuss and pick a casing convention that suits us both. I give zero weight to Rust's casing conventions; as I said, the rigidity of that community does not seem helpful, and we will be writing in husht anyway.

I am confused; if this is internal, why is it `pub`? If it is important that this property never be reflected in a view (i.e., it is not really an intrinsic property of the element, just a contingent value for programming convenience) then there should be a way to enforce that views don't see it. Does Rust have anything like C++ `friend`s or `protected` or something? Ideally, anything that shouldn't be accessible in a view would not be exposed in an interface to the assembly that views can get a hold of. Also, I am guessing that this "index" is the same value that will appear for example in the pair in a Constraint that identifies the elements that are being constrained? If so, then the same typedef should be used here, and the names should be harmonized: if you land on `ElementHandle`, say, then this should be `handle`. And then to soften my first paragraph: maybe we are encouraging views to use these handles for efficiency? Then we should not describe them as "internal" but instead document that they are not intrinsic properties of an element and not to be relied on across a reload of an assembly, but can be used in XXX circumstances and will be invariant over YYY operations, that sort of thing. Oh, and then I see that Assembly has an `elements_by_id` property. Once you've settled on names for the relevant concept, say it's an `ElementHandle`, that should be renamed to something like `handleOfId` or `elementHandleOfId`. Note I have been typing camelCase and your code is using snake_case. The former is more compact, and I am quite used to it. But I am not stuck on it. We should discuss and pick a casing convention that suits us both. I give **zero** weight to Rust's casing conventions; as I said, the rigidity of that community does not seem helpful, and we will be writing in husht anyway.
👍 1
Vectornaut commented 2024-11-11 04:53:35 +00:00
Author
Collaborator
Copy Link

When an assembly with n elements is sent to the engine, each element gets an index from 1 to n, which tells you which column of the configuration matrix it goes in. The index is assigned and used during the realization routine, Assembly::realize; it can change each time realize is called. I've put it in the Element structure, as the index field, because that feels like the most foolproof way to ensure consistent indexing of the configuration matrix and the Gram matrix.

if this is internal, why is it pub?

Good point—it shouldn't be. I made it public in this pull request as a kludge, because I hadn't written a constructor for Element yet. The outline cleanup pull request (#16) introduces an Element constructor, allowing us to make index private with no changes to any other code. I propose making this an issue and then addressing it in #16.

When an assembly with *n* elements is sent to the engine, each element gets an index from 1 to *n*, which tells you which column of the configuration matrix it goes in. The index is assigned and used during the realization routine, `Assembly::realize`; it can change each time `realize` is called. I've put it in the `Element` structure, as the `index` field, because that feels like the most foolproof way to ensure consistent indexing of the configuration matrix and the Gram matrix. > if this is internal, why is it `pub`? Good point—it shouldn't be. I made it public in this pull request as a kludge, because I hadn't written a constructor for `Element` yet. The outline cleanup pull request (#16) introduces an `Element` constructor, allowing us to make `index` private with no changes to any other code. I propose making this an issue and then addressing it in #16.
glen commented 2024-11-11 15:56:35 +00:00
Owner
Copy Link

Fine, given that #16 is around the corner.

Fine, given that #16 is around the corner.
glen marked this conversation as resolved
app-proto/src/assembly.rs Outdated
@ -15,4 +21,4 @@
#[derive(Clone)]
pub struct Constraint {
pub args: (usize, usize),
glen commented 2024-11-10 00:11:01 +00:00
Owner
Copy Link
  • Name choice? These are the two elements constrained by the constraint? We should prefer an entire word or short phrase that makes it clear. constrains would work for me, as would appliesTo. Totally open to other choices.
  • How are constraints that only constrain one element, such as 'the radius of this sphere is 1', represented? This seems to assume that there will be two elements constrained. And as food for thought, we will surely ultimately have user-facing constraints that involve three (maybe more) elements, e.g. 'these three points make an angle of τ/6 radians'. But maybe all such things will be implemented by multiple internal 2-element constraints. Not sure that there is anything to do on that point at the moment, but just wanted to raise the thought.
  • I assume the two usizes are some sort of references to elements. If a primitive type is being used with semantic baggage, typedef it or something like that so that you can write e.g.,

pub constrains: (ElementHandle, ElementHandle)

Again, I am open to other options for the name to use; "ElementRef" would be fine, or you may have another idea. A big part of why to do this is to make development easier should it become convenient to use a different primitive type to encode a reference to an element.

* Name choice? These are the two elements constrained by the constraint? We should prefer an entire word or short phrase that makes it clear. `constrains` would work for me, as would `appliesTo`. Totally open to other choices. * How are constraints that only constrain one element, such as 'the radius of this sphere is 1', represented? This seems to assume that there will be two elements constrained. And as food for thought, we will surely ultimately have user-facing constraints that involve three (maybe more) elements, e.g. 'these three points make an angle of τ/6 radians'. But maybe all such things will be implemented by multiple internal 2-element constraints. Not sure that there is anything to do on that point at the moment, but just wanted to raise the thought. * I assume the two `usize`s are some sort of references to elements. If a primitive type is being used with semantic baggage, typedef it or something like that so that you can write e.g., `pub constrains: (ElementHandle, ElementHandle)` Again, I am open to other options for the name to use; "ElementRef" would be fine, or you may have another idea. A big part of why to do this is to make development easier should it become convenient to use a different primitive type to encode a reference to an element.
👍 1
Vectornaut commented 2024-11-11 03:45:31 +00:00
Author
Collaborator
Copy Link

Name choice? These are the two elements constrained by the constraint? We should prefer an entire word or short phrase that makes it clear. constrains would work for me, as would appliesTo. Totally open to other choices.

I've switched to subjects (in commit ed1890b), as in the phrase "[...] subject to the constraint that [...]." Is that a fitting name? I like to name non-boolean variables with nouns, rather than prepositional verbs like "applies to."

> Name choice? These are the two elements constrained by the constraint? We should prefer an entire word or short phrase that makes it clear. constrains would work for me, as would appliesTo. Totally open to other choices. I've switched to `subjects` (in commit ed1890b), as in the phrase "[...] subject to the constraint that [...]." Is that a fitting name? I like to name non-boolean variables with nouns, rather than prepositional verbs like "applies to."
Vectornaut commented 2024-11-11 03:58:27 +00:00
Author
Collaborator
Copy Link

This seems to assume that there will be two elements constrained.

Right now, there's only one kind of element (a sphere) and one kind of constraint (fixing the generalized angle between two spheres), so the Element and Constraint structures are written pretty narrowly. As we extend dyna3 to more general problems, I'm planning to generalize Element and Constraint as needed—maybe by turning each of them into a trait implemented by a bunch of different structures.

> This seems to assume that there will be two elements constrained. Right now, there's only one kind of element (a sphere) and one kind of constraint (fixing the generalized angle between two spheres), so the `Element` and `Constraint` structures are written pretty narrowly. As we extend dyna3 to more general problems, I'm planning to generalize `Element` and `Constraint` as needed—maybe by turning each of them into a trait implemented by a bunch of different structures.
Vectornaut commented 2024-11-11 04:17:13 +00:00
Author
Collaborator
Copy Link

I assume the two usizes are some sort of references to elements. If a primitive type is being used with semantic baggage, typedef it or something like that so that you can write e.g.,

pub constrains: (ElementHandle, ElementHandle)

Done as soon as I update the PR (in commit ced001b). I've chosen ElementKey and ConstraintKey, since the Rust collections I've seen tend to use the language of keys and values.

> I assume the two `usize`s are some sort of references to elements. If a primitive type is being used with semantic baggage, typedef it or something like that so that you can write e.g., > > `pub constrains: (ElementHandle, ElementHandle)` Done as soon as I update the PR (in commit ced001b). I've chosen `ElementKey` and `ConstraintKey`, since the Rust collections I've seen tend to use the language of keys and values.
glen marked this conversation as resolved
app-proto/src/assembly.rs Outdated
@ -17,3 +23,3 @@
pub struct Constraint {
pub args: (usize, usize),
pub rep: f64,
pub rep: Signal<f64>,
glen commented 2024-11-09 23:53:39 +00:00
Owner
Copy Link

Let's get into a habit of using full words in interfaces/public property names. Looking at this, I don't know if "rep" is a representation, the number of repetitions, something that is being reported, or what... Please rename this suite of properties. Thanks!

Let's get into a habit of using full words in interfaces/public property names. Looking at this, I don't know if "rep" is a representation, the number of repetitions, something that is being reported, or what... Please rename this suite of properties. Thanks!
👍 1
Vectornaut commented 2024-11-11 08:30:32 +00:00
Author
Collaborator
Copy Link

I've replaced rep with lorentz_prod in all the Constraint field names (commit 5839882). We can streamline the names later if they get too unwieldy.

I've replaced `rep` with `lorentz_prod` in all the `Constraint` field names (commit 5839882). We can streamline the names later if they get too unwieldy.
glen marked this conversation as resolved
app-proto/src/assembly.rs Outdated
@ -79,2 +89,3 @@
rep: DVector::<f64>::from_column_slice(&[0.0, 0.0, 0.0, 0.5, -0.5]),
constraints: BTreeSet::default()
constraints: BTreeSet::default(),
index: 0
glen commented 2024-11-10 00:32:03 +00:00
Owner
Copy Link

I am very confused by index: 0. I guess I was wrong that the index is a quick way to get at the element within the assembly? Maybe this will all be clear once the naming of types and properties is a bit improved, but if not, please comment this line.

I am very confused by `index: 0`. I guess I was wrong that the index is a quick way to get at the element within the assembly? Maybe this will all be clear once the naming of types and properties is a bit improved, but if not, please comment this line.
👍 1
Vectornaut commented 2024-11-11 05:08:27 +00:00
Author
Collaborator
Copy Link

The PR update will add a comment that explains the index field's meaning, as described in this comment, and reminds us to make index private when that becomes possible (for example, in pull request #16).

The PR update will add a comment that explains the `index` field's meaning, as described in [this comment](https://code.studioinfinity.org/glen/dyna3/pulls/15#issuecomment-1816), and reminds us to make `index` private when that becomes possible (for example, in pull request #16).
glen commented 2024-11-11 16:14:17 +00:00
Owner
Copy Link

Great. And please add to #16 to rename this property to something that has "column" in it -- that's much more informative than index, which could be an index into anything.

Great. And please add to #16 to rename this property to something that has "column" in it -- that's much more informative than index, which could be an index into anything.
glen marked this conversation as resolved
app-proto/src/assembly.rs Outdated
@ -92,0 +146,4 @@
/* DEBUG */
// log the initial configuration matrix
console::log_1(&JsValue::from("Old configuration:"));
glen commented 2024-11-10 00:35:53 +00:00
Owner
Copy Link

Do you want to set up now a logging/debugging system so that we can leave messages like this in/out at compile time (so that they have no runtime impact when left out), and so always leave them in the code in the repo? Or do you think the project is premature for that, we will plan to take these out in the fairly near future? Either way is fine, just let me know; I am just making the point that we definitely don't want any raw console logs in our standard code, ultimately.

Do you want to set up now a logging/debugging system so that we can leave messages like this in/out at compile time (so that they have no runtime impact when left out), and so always leave them in the code in the repo? Or do you think the project is premature for that, we will plan to take these out in the fairly near future? Either way is fine, just let me know; I am just making the point that we definitely don't want any raw console logs in our standard code, ultimately.
👍 1
Vectornaut commented 2024-11-11 05:33:20 +00:00
Author
Collaborator
Copy Link

I think it's too early to design a formal logging system. I've been adding and removing log messages pretty freely as I test new features and track down bugs. I just started a wiki page where we can jot down log messages that come in handy; we can refer to that list when we start thinking about how to log stuff more formally.

I think it's too early to design a formal logging system. I've been adding and removing log messages pretty freely as I test new features and track down bugs. I just started a [wiki page](wiki/Logging) where we can jot down log messages that come in handy; we can refer to that list when we start thinking about how to log stuff more formally.
glen marked this conversation as resolved
app-proto/src/assembly.rs
@ -92,0 +171,4 @@
}
));
console::log_2(&JsValue::from("Steps:"), &JsValue::from(history.scaled_loss.len() - 1));
console::log_2(&JsValue::from("Loss:"), &JsValue::from(*history.scaled_loss.last().unwrap()));
glen commented 2024-11-10 00:37:02 +00:00
Owner
Copy Link

Are the _1 and _2 suffixes the number of arguments? I take it rust doesn't have functions with a varying number of arguments? Or do they mean something else?

Are the _1 and _2 suffixes the number of arguments? I take it rust doesn't have functions with a varying number of arguments? Or do they mean something else?
👍 1
Vectornaut commented 2024-11-11 05:37:26 +00:00
Author
Collaborator
Copy Link

Are the _1 and _2 suffixes the number of arguments?

Yes—log_0 through log_7 are convenience wrappers for log, which takes an array of things to log.

> Are the _1 and _2 suffixes the number of arguments? Yes—[`log_0`](https://docs.rs/web-sys/latest/web_sys/console/fn.log_0.html) through [`log_7`](https://docs.rs/web-sys/latest/web_sys/console/fn.log_7.html) are convenience wrappers for [`log`](https://docs.rs/web-sys/latest/web_sys/console/fn.log.html), which takes an array of things to log.
glen commented 2024-11-11 16:12:09 +00:00
Owner
Copy Link

Is that a Rustism? Or a local convenience? It's pretty awful; I hope we can avoid it in the long run, one way or another.

Is that a Rustism? Or a local convenience? It's pretty awful; I hope we can avoid it in the long run, one way or another.
glen marked this conversation as resolved
app-proto/src/engine.rs
@ -27,0 +120,4 @@
0.0, 0.0, 0.0, -2.0, 0.0
]);
}
glen commented 2024-11-10 00:47:05 +00:00
Owner
Copy Link

The coordinates are reordered from the notes, I believe, with the "position" coordinates first, and then the bend & coradius. And the sign is flipped from that Q, right? But the relative scaling of the position and radius terms seems strange to me; in the notes it's (cr/2 + rc/2) - xx - yy - zz; here it seems to be xx + yy + zz - 2cr -2rc. I know we discussed at one point what effect that relative factor of 4 between the radii and position terms has on all of the claims in the inversive notes; my apologies for having forgotten/still being confused on the details. Please put some comments either here or in the notes file on this topic to clarify how the actual coordinates we are using compare to the ones in the inversive notes. Thanks!

The coordinates are reordered from the notes, I believe, with the "position" coordinates first, and then the bend & coradius. And the sign is flipped from that Q, right? But the relative scaling of the position and radius terms seems strange to me; in the notes it's (cr/2 + rc/2) - xx - yy - zz; here it seems to be xx + yy + zz - 2cr -2rc. I know we discussed at one point what effect that relative factor of 4 between the radii and position terms has on all of the claims in the inversive notes; my apologies for having forgotten/still being confused on the details. Please put some comments either here or in the notes file on this topic to clarify how the _actual_ coordinates we are using compare to the ones in the inversive notes. Thanks!
👍 1
Vectornaut commented 2024-11-11 07:57:48 +00:00
Author
Collaborator
Copy Link

I've added a section about this to the notes!

I've added a section about this to the notes!
glen commented 2024-11-11 16:28:22 +00:00
Owner
Copy Link

Ah, that's very helpful. I had forgotten that the coordinates changed, so that the Lorentz product only flipped sign, so that all of the claims about the values of the Lorentz product in the notes are still true for Q' just with the signs flipped.

Just remember that at whatever point we add a "radius" constraint to a sphere, that the constraint itself is represented by the radius, which is the salient platonic characteristic of that constraint; the fact that the constraint is achieved by setting some coordinate matrix entry to 1/2r is secondary.

In that vein, we should both be clear about the fact that currently a Constraint is represented by "set the Gram matrix entry at this position to blah" is temporary, for this PR and the run-up to definitely settling on a first-effort engine. That's not a proper part of the platonic reality being modeled; it wouldn't be preserved across the switch to a totally different representation/optimization method, which is a good test for what could ultimately be in the Assembly. So as soon as we have decided that this is the engine we're going to go with for "ver 0.1", so to speak, we need an issue to replace the sort of Constraint that's there now with platonically viable ones: "angle between elements", "elements are tangent", "elements are parallel", "point lies on element", etc. etc.; these then get translated in the engine to the forms in which they can be practically enforced in that particular engine.

Ah, that's very helpful. I had forgotten that the coordinates changed, so that the Lorentz product only flipped sign, so that all of the claims about the values of the Lorentz product in the notes are still true for $Q'$ just with the signs flipped. Just remember that at whatever point we add a "radius" constraint to a sphere, that the _constraint_ itself is represented by the radius, which is the salient platonic characteristic of that constraint; the fact that the constraint is _achieved_ by setting some coordinate matrix entry to 1/2r is secondary. In that vein, we should both be clear about the fact that currently a Constraint is represented by "set the Gram matrix entry at this position to blah" is temporary, for this PR and the run-up to definitely settling on a first-effort engine. That's not a proper part of the platonic reality being modeled; it wouldn't be preserved across the switch to a totally different representation/optimization method, which is a good test for what could ultimately be in the Assembly. So as soon as we have decided that this is the engine we're going to go with for "ver 0.1", so to speak, we need an issue to replace the sort of Constraint that's there now with platonically viable ones: "angle between elements", "elements are tangent", "elements are parallel", "point lies on element", etc. etc.; these then get translated in the engine to the forms in which they can be practically enforced in that particular engine.
glen marked this conversation as resolved
app-proto/src/engine.rs
@ -27,0 +277,4 @@
// --- tests ---
#[cfg(test)]
mod tests {
glen commented 2024-11-10 00:48:01 +00:00
Owner
Copy Link

love that there are tests right in here, we definitely want to keep that up and have them be as pervasive as possible.

love that there are tests right in here, we definitely want to keep that up and have them be as pervasive as possible.
👍 1
Vectornaut commented 2024-11-11 05:39:58 +00:00
Author
Collaborator
Copy Link

Yes, this is one of the recommended ways to organize tests in Rust!

Yes, this is one of the [recommended ways](https://doc.rust-lang.org/book/ch11-03-test-organization.html) to organize tests in Rust!
glen marked this conversation as resolved
app-proto/src/outline.rs
@ -1,12 +1,40 @@
use itertools::Itertools;
glen commented 2024-11-10 00:49:38 +00:00
Owner
Copy Link

Definitely at some point we are going to need to separate the src directory into parts for the Assembly/Engine and for each of the views, but fine if you don't think that's warranted yet. On the other hand, if you just want to set up a reasonable hierarchy now and see how it works, that's fine too.

Definitely at some point we are going to need to separate the src directory into parts for the Assembly/Engine and for each of the views, but fine if you don't think that's warranted yet. On the other hand, if you just want to set up a reasonable hierarchy now and see how it works, that's fine too.
👍 1
Vectornaut commented 2024-11-11 05:49:13 +00:00
Author
Collaborator
Copy Link

Definitely at some point we are going to need to separate the src directory into parts for the Assembly/Engine [...]

Yeah, I think we'll probably want separate modules for "front of house" and "back of house" code at some point. Right now it does seem like the engine and assembly modules should go together in the "back of house" module, but I could imagine that changing, since the assembly structure is pretty tailored to the needs of the user interface.

[...] and for each of the views

Note that the Display and Outline views each have their own module already: that's why the expressions mod display; and mod outline; appear at the top of main.rs.

> Definitely at some point we are going to need to separate the src directory into parts for the Assembly/Engine [...] Yeah, I think we'll probably want separate modules for "front of house" and "back of house" code at some point. Right now it does seem like the `engine` and `assembly` modules should go together in the "back of house" module, but I could imagine that changing, since the assembly structure is pretty tailored to the needs of the user interface. > [...] and for each of the views Note that the `Display` and `Outline` views each have their own module already: that's why the expressions `mod display;` and `mod outline;` appear at the top of `main.rs`.
glen commented 2024-11-11 15:03:33 +00:00
Owner
Copy Link

I was commenting on the fact that outline is in the same directory as assembly and engine. I guess the module structure may not be immediately clear to the untrained eye; but directory structure is.

Also be cautious about the notion that "assembly is tailored to needs of UI". The driver for what goes in Assembly is the facts of the platonic universe we are observing. So color and label are OK because the sphere labeled George is red in the platonic universe. But unless "hidden" means that an entity must be hidden in all possible views -- and I doubt it should -- that should not be part of Assembly. (That's just an example, I know there is not a "hidden" property yet.)

I was commenting on the fact that outline is in the same directory as assembly and engine. I guess the module structure may not be immediately clear to the untrained eye; but directory structure is. Also be cautious about the notion that "assembly is tailored to needs of UI". The driver for what goes in Assembly is the facts of the platonic universe we are observing. So color and label are OK because the sphere labeled George is red in the platonic universe. But unless "hidden" means that an entity must be hidden in all possible views -- and I doubt it should -- that should not be part of Assembly. (That's just an example, I know there is not a "hidden" property yet.)
glen marked this conversation as resolved
Vectornaut commented 2024-11-11 00:50:16 +00:00
Author
Collaborator
Copy Link

Please fix the warning

Done—by compiling engine::point only in test mode, where it's already in use. I'll update the PR once I do the other requested changes.

if there is a compiler flag to make warnings into errors, please turn it on

These posts describe some drawbacks of and alternatives to making the build fail whenever there's a warning:

To me, it seems more convenient to just adopt the policy that code must compile without warnings to be merged into main.

> Please fix the warning Done—by compiling `engine::point` only in test mode, where it's already in use. I'll update the PR once I do the other requested changes. > if there is a compiler flag to make warnings into errors, please turn it on These posts describe some drawbacks of and alternatives to making the build fail whenever there's a warning: - [Rust Design Anti-Patterns: `#![deny(warnings)]`](https://rust-unofficial.github.io/patterns/anti_patterns/deny-warnings.html) - [PSA: `deny(warnings)` is actively harmful](https://www.reddit.com/r/rust/comments/f5xpib/psa_denywarnings_is_actively_harmful/) To me, it seems more convenient to just adopt the policy that code must compile without warnings to be merged into main.
glen commented 2024-11-11 01:28:18 +00:00
Owner
Copy Link

To me, it seems more convenient to just adopt the policy that code must compile without warnings to be merged into main.

OK then we need to prioritize getting the gitea CI running again and add a compile-with-warnings-disallowed CI test. The only real way to maintain a state is to fail at least CI if not every commit if you're not in that state, in my experience.

> To me, it seems more convenient to just adopt the policy that code must compile without warnings to be merged into main. OK then we need to prioritize getting the gitea CI running again and add a compile-with-warnings-disallowed CI test. The only real way to maintain a state is to fail at least CI if not every commit if you're not in that state, in my experience.
Vectornaut commented 2024-11-11 01:47:07 +00:00
Author
Collaborator
Copy Link

Are they theoretically identical, with differences only occurring for numerical reasons?

Yes. In both cases, we're trying to solve a problem like

\left[\begin{matrix} 1 & 0 \\ 0 & 0 \end{matrix}\right] \left[\begin{matrix} a & b \\ c & d \end{matrix}\right] \left[\begin{matrix} x \\ 0 \end{matrix}\right] = \left[\begin{matrix} s \\ 0 \end{matrix}\right],

where x is the unknown. The original method was to solve the "collapsed" equation

\left[\begin{matrix} a \end{matrix}\right] \left[\begin{matrix} x \end{matrix}\right] = \left[\begin{matrix} s \end{matrix}\right].

The new method is to solve the modified equation

\left[\begin{matrix} a & 0 \\ 0 & 1 \end{matrix}\right] \left[\begin{matrix} x \\ y \end{matrix}\right] = \left[\begin{matrix} s \\ 0 \end{matrix}\right],

which is equivalent from an exact point of view.

Is it the case that you don't get exact 0s in the resulting Newton step with the adjusted approach in Rust, only approximate 0s?

In Julia, the new method correctly sets the frozen entries of the Newton step to exactly zero. You can verify this by launching the Julia REPL from engine-proto/gram-test and then calling

include("irisawa-hexlet.jl")
all(A[4,1:4] == [0, 0, 0, 0] for A in history_alt.base_step)

If you'd like, I could write a test to do the same verification in Rust.

I assume any time lost in doing the matrix reconfiguration is gained back in dealing with a Hessian with fewer rows/columns...

I wouldn't guess this without profiling. It probably depends on the size of the matrix, and I have the impression that memory allocation tends to be much more costly than floating-point operations.

> Are they theoretically identical, with differences only occurring for numerical reasons? Yes. In both cases, we're trying to solve a problem like $$\left[\begin{matrix} 1 & 0 \\ 0 & 0 \end{matrix}\right] \left[\begin{matrix} a & b \\ c & d \end{matrix}\right] \left[\begin{matrix} x \\ 0 \end{matrix}\right] = \left[\begin{matrix} s \\ 0 \end{matrix}\right],$$ where $x$ is the unknown. The original method was to solve the "collapsed" equation $$\left[\begin{matrix} a \end{matrix}\right] \left[\begin{matrix} x \end{matrix}\right] = \left[\begin{matrix} s \end{matrix}\right].$$ The new method is to solve the modified equation $$\left[\begin{matrix} a & 0 \\ 0 & 1 \end{matrix}\right] \left[\begin{matrix} x \\ y \end{matrix}\right] = \left[\begin{matrix} s \\ 0 \end{matrix}\right],$$ which is equivalent from an exact point of view. > Is it the case that you don't get exact 0s in the resulting Newton step with the adjusted approach in Rust, only approximate 0s? In Julia, the new method correctly sets the frozen entries of the Newton step to exactly zero. You can verify this by launching the Julia REPL from `engine-proto/gram-test` and then calling ``` include("irisawa-hexlet.jl") all(A[4,1:4] == [0, 0, 0, 0] for A in history_alt.base_step) ``` If you'd like, I could write a test to do the same verification in Rust. > I assume any time lost in doing the matrix reconfiguration is gained back in dealing with a Hessian with fewer rows/columns... I wouldn't guess this without profiling. It probably depends on the size of the matrix, and I have the impression that memory allocation tends to be much more costly than floating-point operations.
Vectornaut commented 2024-11-11 01:52:32 +00:00
Author
Collaborator
Copy Link

OK, went through the code. Since this prototype seems like it has a chance to become the start of the actual code, I don't think it's too early to start trying to use some hygienic naming and organization

Yes, engine.rs could definitely evolve into the production engine!

> OK, went through the code. Since this prototype seems like it has a chance to become the start of the actual code, I don't think it's too early to start trying to use some hygienic naming and organization Yes, `engine.rs` could definitely evolve into the production engine!
glen commented 2024-11-11 02:06:53 +00:00
Owner
Copy Link

The new method correctly sets the frozen entries of the Newton step to exactly zero. You can verify this by launching the Julia REPL from engine-proto/gram-test and then calling

include("irisawa-hexlet.jl")
all(A[4,1:4] == [0, 0, 0, 0] for A in history_alt.base_step)

If you'd like, I could write a test to do the same verification in Rust.

Sure, may as well do the verification. I guess then the difference must be in the other coordinates, in the "x" part in your matrix equations above. They must come out microscopically different in the two versions (how else could they diverge from each other?). So I guess that's not really consequential/alarming.

>The new method correctly sets the frozen entries of the Newton step to exactly zero. You can verify this by launching the Julia REPL from engine-proto/gram-test and then calling ``` include("irisawa-hexlet.jl") all(A[4,1:4] == [0, 0, 0, 0] for A in history_alt.base_step) ``` > If you'd like, I could write a test to do the same verification in Rust. Sure, may as well do the verification. I guess then the difference must be in the other coordinates, in the "x" part in your matrix equations above. They must come out microscopically different in the two versions (how else could they diverge from each other?). So I guess that's not really consequential/alarming.
Vectornaut commented 2024-11-11 02:13:05 +00:00
Author
Collaborator
Copy Link

If you go into engine-proto/gram-test, launch Julia, and then include("irisawa-hexlet.jl"), you should see that it prints basically the same loss history until the last few steps, when the lower default precision of the Rust engine really starts to show.

For me, the Julia file does not print a loss history, only the final loss

Oh, right: in the Julia version, you have to print the loss history manually. I've updated the PR to reflect this.

>> If you go into `engine-proto/gram-test`, launch Julia, and then `include("irisawa-hexlet.jl")`, you should see that it prints basically the same loss history until the last few steps, when the lower default precision of the Rust engine really starts to show. > > For me, the Julia file does not print a loss history, only the final loss Oh, right: in the Julia version, you have to print the loss history manually. I've updated the PR to reflect this.
Vectornaut added 6 commits 2024-11-11 07:50:14 +00:00 
This function will eventually be used in the application, but right now
it's only used in tests.
This will make it easier to change the key types if we change how we
store and access elements and constraints.
Also, remind us to make the field private when that becomes possible.
Vectornaut added 1 commit 2024-11-11 07:56:30 +00:00
Vectornaut added 1 commit 2024-11-11 08:05:55 +00:00
Vectornaut commented 2024-11-11 08:09:47 +00:00
Author
Collaborator
Copy Link

Pull request updated and ready for more review! I think I've made all of the changes you requested; let me know if I missed anything.

Pull request updated and ready for more review! I think I've made all of the changes you requested; let me know if I missed anything.
Vectornaut added 1 commit 2024-11-11 08:31:13 +00:00
glen reviewed 2024-11-11 16:06:56 +00:00
app-proto/src/assembly.rs
@ -19,1 +34,3 @@
pub rep: f64,
pub subjects: (ElementKey, ElementKey),
pub lorentz_prod: Signal<f64>,
pub lorentz_prod_text: Signal<String>,
glen commented 2024-11-11 16:06:56 +00:00
Owner
Copy Link

But now I am confused; is this the string like 0.5 that represents the specified lorentz_prod of the constraint in the outline view? Why is that in the assembly? Or am I misunderstanding the import of this property?

If not, it seems like a violation of separation of concerns. The platonic reality of a constraint is just the number that the lorentz product has to be. The string is just an artifact of the outline view, so it should live there. To drive that point home more, we will surely at some point want to represent Constraints in the Display view. There, we might represent them as little translucent angle symbols, perhaps labeled with the angle in radians or radians/tau. It's unlikely that the raw lorentz product would appear in the graphical view. So an altogether different string would be involved.

But now I am confused; is this the string like 0.5 that represents the specified lorentz_prod of the constraint in the outline view? Why is that in the assembly? Or am I misunderstanding the import of this property? If not, it seems like a violation of separation of concerns. The platonic reality of a constraint is just the number that the lorentz product has to be. The string is just an artifact of the outline view, so it should live there. To drive that point home more, we will surely at some point want to represent Constraints in the Display view. There, we might represent them as little translucent angle symbols, perhaps labeled with the angle in radians or radians/tau. It's unlikely that the raw lorentz product would appear in the graphical view. So an altogether different string would be involved.
Vectornaut commented 2024-11-11 21:58:30 +00:00
Author
Collaborator
Copy Link

is this the string like 0.5 that represents the specified lorentz_prod of the constraint in the outline view?

Yes. More generally, it's the reactive string that represents the Lorentz product in every view where it's edited or displayed as a string. Right now, this just means the two inputs that appear in the outline view, but in the future it might also include inputs in a Gram matrix view.

I agree that the model and view aspects of the Lorentz product should be separated better. One way might be to write a wrapper around Constraint to hold reactive values which need to stay synchronized across several views, but aren't part of the Platonic ideal assembly. However, I'd expect this way—or any other way—to lead to an extra layer of organization over the whole assembly. For that reason, I think that any attempt to do this should be its own dedicated pull request. Right now, I don't see the less-than-ideal organization as a major obstacle to developing a minimum viable product.

> is this the string like 0.5 that represents the specified lorentz_prod of the constraint in the outline view? Yes. More generally, it's the reactive string that represents the Lorentz product in every view where it's edited or displayed as a string. Right now, this just means the two inputs that appear in the outline view, but in the future it might also include inputs in a Gram matrix view. I agree that the model and view aspects of the Lorentz product should be separated better. One way might be to write a wrapper around `Constraint` to hold reactive values which need to stay synchronized across several views, but aren't part of the Platonic ideal assembly. However, I'd expect this way—or any other way—to lead to an extra layer of organization over the whole assembly. For that reason, I think that any attempt to do this should be its own dedicated pull request. Right now, I don't see the less-than-ideal organization as a major obstacle to developing a minimum viable product.
glen commented 2024-11-11 22:44:28 +00:00
Owner
Copy Link

Especially right now where the only occurrences of the string are in the outline view, it seems absolutely clear that this string (and its validity) should be housed in the view, not in the assembly. It should then be "wired up" so that when a valid string has been entered, that updates the actual constraint in the assembly; and vice-versa, if the actual constraint in the assembly becomes changed (by some other control, say), then the representing view-string should be updated to the string of the new value, and the representing validity-flag should be updated to true.

Just like in Numberscope when you enter invalid input for a parameter, it should not disable the associated entity, it should simply not (yet at that point) update it (so the last valid value will continue to be used, until there's a new valid input in the box). There should of course be some visible indication that the input is in a bad state; if you think it is important enough, that indication could in addition display that previous value that's still being used.

With this design, suppose there is a Gram view in the future. It has its input boxes with their reactive strings. When one of them is updated, that action will update the actual constraint in the Assembly. And that event in turn should update the reactive string in the Outline view. So everything remains harmonized. This is no different from dragging a point onto an entity in the Display to mean that it should have a new constraint of lying on that entity vs selecting the point and entity in the Outline view and hitting the "coincident" tool, or whatever. It's the type of thing that will come up over and over again as we create multiple ways of doing things that amount to the same action in the Assembly -- there will not be any synchronized state kept across views. They should synchronize by virtue of two-way communication with the Assembly.

I think it's important to do it this way, because if you think about actual use cases, I think it will be quite rare that there would really ever be a "reactive string that needs to stay synchronized across views". For example, as dyna3 matures, the outline view is going to express this sort of a constraint as an angle, and it will likely have a degrees or radians or radians/tau selector which will affect what string is used, while the Gram matrix will likely express it as the cosine-value of the angle, which will entail a different string.

For all these reasons, please do change the location/architecture of the reactive values that are not properly part of the model (Assembly) for this PR. Let's keep the models and the view-controllers well separated from the start. Thanks so much for your understanding!

Note that if there is a concept in the Assembly of a "turned-off constraint", then the on/off state of a constraint should be represented as such in the model, and views that display or allow control of that bit should have their needed apparatus to do so.

Especially right now where the only occurrences of the string are in the outline view, it seems absolutely clear that this string (and its validity) should be housed in the view, not in the assembly. It should then be "wired up" so that when a valid string has been entered, that updates the actual constraint in the assembly; and vice-versa, if the actual constraint in the assembly becomes changed (by some other control, say), then the representing view-string should be updated to the string of the new value, and the representing validity-flag should be updated to true. Just like in Numberscope when you enter invalid input for a parameter, it should not disable the associated entity, it should simply not (yet at that point) update it (so the last valid value will continue to be used, until there's a new valid input in the box). There should of course be some visible indication that the input is in a bad state; if you think it is important enough, that indication could in addition display that previous value that's still being used. With this design, suppose there is a Gram view in the future. It has its input boxes with their reactive strings. When one of them is updated, that action will update the actual constraint in the Assembly. And that event in turn should update the reactive string in the Outline view. So everything remains harmonized. This is no different from dragging a point onto an entity in the Display to mean that it should have a new constraint of lying on that entity vs selecting the point and entity in the Outline view and hitting the "coincident" tool, or whatever. It's the type of thing that will come up over and over again as we create multiple ways of doing things that amount to the same action in the Assembly -- there will not be any synchronized state kept across views. They should synchronize by virtue of two-way communication with the Assembly. I think it's important to do it this way, because if you think about actual use cases, I think it will be quite rare that there would really ever be a "reactive string that needs to stay synchronized across views". For example, as dyna3 matures, the outline view is going to express this sort of a constraint as an angle, and it will likely have a degrees or radians or radians/tau selector which will affect what string is used, while the Gram matrix will likely express it as the cosine-value of the angle, which will entail a _different_ string. For all these reasons, please do change the location/architecture of the reactive values that are not properly part of the model (Assembly) for this PR. Let's keep the models and the view-controllers well separated from the start. Thanks so much for your understanding! Note that if there is a concept in the Assembly of a "turned-off constraint", then the on/off state of a constraint should be represented as such in the model, and views that display or allow control of that bit should have their needed apparatus to do so.
Vectornaut commented 2024-11-12 00:26:51 +00:00
Author
Collaborator
Copy Link

Just like in Numberscope when you enter invalid input for a parameter, it should not disable the associated entity, it should simply not (yet at that point) update it (so the last valid value will continue to be used, until there's a new valid input in the box). There should of course be some visible indication that the input is in a bad state; if you think it is important enough, that indication could in addition display that previous value that's still being used.

With this design, suppose there is a Gram view in the future. It has its input boxes with their reactive strings. When one of them is updated, that action will update the actual constraint in the Assembly. And that event in turn should update the reactive string in the Outline view. So everything remains harmonized.

That seems workable. I could probably implement it by moving the Lorentz product string into the LorentzProductInput component introduced in pull request #16.

For all these reasons, please do change the location/architecture of the reactive values that are not properly part of the model (Assembly) for this PR.

The likely costs of this will be:

  • The work I do to get this feature into this PR will be partly or entirely scrapped and redone for PR #16.
  • Merging #16 will take longer because of the additional merge conflicts.

Let me know whether you'd still like me to go ahead.

> Just like in Numberscope when you enter invalid input for a parameter, it should not disable the associated entity, it should simply not (yet at that point) update it (so the last valid value will continue to be used, until there's a new valid input in the box). There should of course be some visible indication that the input is in a bad state; if you think it is important enough, that indication could in addition display that previous value that's still being used. > With this design, suppose there is a Gram view in the future. It has its input boxes with their reactive strings. When one of them is updated, that action will update the actual constraint in the Assembly. And that event in turn should update the reactive string in the Outline view. So everything remains harmonized. That seems workable. I could probably implement it by moving the Lorentz product string into the `LorentzProductInput` component introduced in pull request #16. > For all these reasons, please do change the location/architecture of the reactive values that are not properly part of the model (Assembly) for this PR. The likely costs of this will be: - The work I do to get this feature into this PR will be partly or entirely scrapped and redone for PR #16. - Merging #16 will take longer because of the additional merge conflicts. Let me know whether you'd still like me to go ahead.
glen commented 2024-11-12 00:45:40 +00:00
Owner
Copy Link

Please file an issue to do this immediately after #16.

Please file an issue to do this immediately after #16.
glen marked this conversation as resolved
glen reviewed 2024-11-11 16:08:39 +00:00
app-proto/src/assembly.rs
@ -20,0 +34,4 @@
pub subjects: (ElementKey, ElementKey),
pub lorentz_prod: Signal<f64>,
pub lorentz_prod_text: Signal<String>,
pub lorentz_prod_valid: Signal<bool>,
glen commented 2024-11-11 16:08:39 +00:00
Owner
Copy Link

It's unclear what it means for a lorentz_product to be "valid"

This signal keeps track of whether lorentz_prod_text can be parsed to a floating point number. Maybe the name lorentz_prod_specified would be more descriptive?

The realization routine only enforces a constraint when lorentz_prod_valid is true. This helps ensure that the engine is always enforcing the constraints that the user is looking at. In pull request #16, the interface flags constraints that aren't being enforced because the Lorentz product can't be parsed.

Like lorentz_prod_text, this signal is closer to the views than to the model, so the discussion about separation of concerns from this comment applies to it too.

> It's unclear what it means for a lorentz_product to be "valid" This signal keeps track of whether `lorentz_prod_text` can be parsed to a floating point number. Maybe the name `lorentz_prod_specified` would be more descriptive? The realization routine only enforces a constraint when `lorentz_prod_valid` is true. This helps ensure that the engine is always enforcing the constraints that the user is looking at. In pull request #16, the interface flags constraints that aren't being enforced because the Lorentz product can't be parsed. Like `lorentz_prod_text`, this signal is closer to the views than to the model, so the discussion about separation of concerns from [this comment](https://code.studioinfinity.org/glen/dyna3/pulls/15#issuecomment-1851) applies to it too.
glen commented 2024-11-11 22:45:40 +00:00
Owner
Copy Link

Yes, I think this will naturally go somewhere else and be called something clearer when the reactive strings that are part of the view go into the view.

Yes, I think this will naturally go somewhere else and be called something clearer when the reactive strings that are part of the view go into the view.
glen marked this conversation as resolved
glen commented 2024-11-11 16:37:00 +00:00
Owner
Copy Link

Sure, may as well do the verification.

Sorry, to be clearer: for this PR, please add a test that verifies, in some reasonably nontrivial case, that when the row/col of a frozen coordinate is set to 00100, the step comes out precisely 0 in that coordinate; I don't see one. Thanks!

> Sure, may as well do the verification. Sorry, to be clearer: for this PR, please add a test that verifies, in some reasonably nontrivial case, that when the row/col of a frozen coordinate is set to 00100, the step comes out precisely 0 in that coordinate; I don't see one. Thanks!
👍 1
glen commented 2024-11-11 16:38:37 +00:00
Owner
Copy Link

Thanks for all of your efforts on this! I think we are close on this one; just the three items from the three comments immediately above, then final review.

Thanks for all of your efforts on this! I think we are close on this one; just the three items from the three comments immediately above, then final review.
glen commented 2024-11-11 19:46:37 +00:00
Owner
Copy Link

Oh and I guess there's one more item: putting engine/assembly in a different directory from views. That one is not critical at this point, since there aren't all that many source files yet, but you should totally feel free to do it at any time to foster a good structure going forward.

Oh and I guess there's one more item: putting engine/assembly in a different directory from views. That one is not critical at this point, since there aren't all that many source files yet, but you should totally feel free to do it at any time to foster a good structure going forward.
👍 1
Vectornaut added 2 commits 2024-11-11 23:45:48 +00:00
Vectornaut commented 2024-11-11 23:47:22 +00:00
Author
Collaborator
Copy Link

for this PR, please add a test that verifies, in some reasonably nontrivial case, that when the row/col of a frozen coordinate is set to 00100, the step comes out precisely 0 in that coordinate; I don't see one. Thanks!

Done with frozen_entry_test, added in commit 5332fda.

> for this PR, please add a test that verifies, in some reasonably nontrivial case, that when the row/col of a frozen coordinate is set to 00100, the step comes out precisely 0 in that coordinate; I don't see one. Thanks! Done with `frozen_entry_test`, added in commit 5332fda.
Vectornaut commented 2024-11-11 23:51:30 +00:00
Author
Collaborator
Copy Link

Oh and I guess there's one more item: putting engine/assembly in a different directory from views. That one is not critical at this point

To simplify future merges, I'd prefer to do this after the pull requests I already have queued up (#16 and then the upcoming cargo-examples pull request).

> Oh and I guess there's one more item: putting engine/assembly in a different directory from views. That one is not critical at this point To simplify future merges, I'd prefer to do this after the pull requests I already have queued up (#16 and then the upcoming [`cargo-examples`](src/branch/cargo-examples) pull request).
glen merged commit 707618cdd3 into main 2024-11-12 00:46:16 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
glen
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
question

More information is needed

  
todo

Planned step in implementation

  
wontfix

This won't be fixed

No Label
bug
duplicate
enhancement
question
todo
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: glen/dyna3#15
No description provided.
Delete Branch "engine-integration"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?