glen/dyna3
1
0
Fork 0
Code Issues 14 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 +5 -1
1 changed files with 5 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit b8ca1139d5 - Show all commits

6
app-proto/src/assembly.rs
View File

@ -19,7 +19,11 @@ pub struct Element {
pub representation: DVector<f64>,
glen marked this conversation as resolved Outdated
glen commented 2024-11-10 00:14:32 +00:00
Outdated
Review
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
Outdated
Review
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
Outdated
Review
Copy Link

Fine, given that #16 is around the corner.

Fine, given that #16 is around the corner.
pub constraints: BTreeSet<ConstraintKey>,
// internal properties, not reflected in any view
// the configuration matrix column index that was assigned to this element
// last time the assembly was realized
/* TO DO */
glen marked this conversation as resolved Outdated
glen commented 2024-11-10 00:11:01 +00:00
Outdated
Review
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
Outdated
Review
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
Outdated
Review
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
Outdated
Review
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.
// this is public, as a kludge, because `Element` doesn't have a constructor
glen marked this conversation as resolved Outdated
glen commented 2024-11-09 23:53:39 +00:00
Outdated
Review
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
Outdated
Review
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.
// yet. it should be made private as soon as the constructor is written
pub index: usize
}