Generalize constraints to observables #48
|
|
@ -127,7 +127,7 @@ details[open]:has(li) .element-switch::after {
|
|||
font-style: italic;
|
||||
}
|
||||
|
||||
.observable.invalid {
|
||||
.observable.invalid-constraint {
|
||||
color: var(--text-invalid);
|
||||
}
|
||||
|
||||
|
|
@ -138,7 +138,7 @@ details[open]:has(li) .element-switch::after {
|
|||
border-radius: 2px;
|
||||
}
|
||||
|
||||
.observable.invalid > input[type=text] {
|
||||
.observable.invalid-constraint > input[type=text] {
|
||||
border-color: var(--border-invalid);
|
||||
}
|
||||
|
||||
|
|
@ -150,11 +150,11 @@ details[open]:has(li) .element-switch::after {
|
|||
font-style: normal;
|
||||
}
|
||||
|
||||
.constrained > .status::after, details:has(.constrained):not([open]) .status::after {
|
||||
.valid-constraint > .status::after, details:has(.valid-constraint):not([open]) .status::after {
|
||||
content: '🔗';
|
||||
}
|
||||
|
||||
.invalid > .status::after, details:has(.invalid):not([open]) .status::after {
|
||||
.invalid-constraint > .status::after, details:has(.invalid-constraint):not([open]) .status::after {
|
||||
content: '⚠';
|
||||
color: var(--text-invalid);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -210,7 +210,7 @@ pub fn AddRemove() -> View {
|
|||
}
|
||||
);
|
||||
let desired = create_signal(0.0);
|
||||
let role = create_signal(ObservableRole::Measure);
|
||||
let role = create_signal(ObservableRole::Measurement);
|
||||
state.assembly.insert_observable(Observable {
|
||||
subjects: subjects,
|
||||
measured: measured,
|
||||
|
|
@ -242,7 +242,7 @@ pub fn AddRemove() -> View {
|
|||
format!("Updated constraint with subjects ({}, {})", subjects.0, subjects.1)
|
||||
));
|
||||
desired.track();
|
||||
if role.with(|r| matches!(r, ObservableRole::Constrain)) {
|
||||
if role.with(|rl| rl.is_valid_constraint()) {
|
||||
state.assembly.realize();
|
||||
}
|
||||
});
|
||||
|
|
|
|||
|
|
@ -112,9 +112,17 @@ impl Element {
|
|||
}
|
||||
|
||||
pub enum ObservableRole {
|
||||
Measure,
|
||||
Constrain,
|
||||
Invalid
|
||||
Measurement,
|
||||
Constraint(bool)
|
||||
}
|
||||
|
||||
impl ObservableRole {
|
||||
pub fn is_valid_constraint(&self) -> bool {
|
||||
|
glen marked this conversation as resolved
Outdated
|
||||
match self {
|
||||
ObservableRole::Measurement => false,
|
||||
ObservableRole::Constraint(valid) => *valid
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
|
|
@ -126,6 +134,12 @@ pub struct Observable {
|
|||
pub role: Signal<ObservableRole>
|
||||
}
|
||||
|
||||
impl Observable {
|
||||
fn role_is_valid_constraint_untracked(&self) -> bool {
|
||||
self.role.with_untracked(|role| role.is_valid_constraint())
|
||||
}
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
Note here we are baking into this design the notion that the only specification of an Absent SpecifiedValue that we ever care to remember is Note here we are baking into this design the notion that the only specification of an Absent SpecifiedValue that we ever care to remember is `''`. Is that OK? I worry in particular that when we want to allow disabling while remembering the un-disabled spec, this could bite us. Or maybe in that situation, the bit as to whether a regulator is active goes into the Regulator, and an inactive Regulator with a Present SpecifiedValue represents that state. So this is probably OK, but thought I'd just raise the point before we merge this.
Vectornaut
commented
I definitely think we should give the I definitely think we should give the `Absent` variant a specification field if we end up with more than one way to specify absence. However, I'm not sure it makes sense to add that field now. It feels weird to have a field whose value should always be an empty string.
glen
commented
I think it would be weirder to have -- I mean, the spec should be the spec and stored in the same place whether it corresponds to an absent or a present SpecifiedValue. So if you think that's likely where we're headed, we should redesign this now, shouldn't we? Thoughts? I think it would be weirder to have
```
pub enum SpecifiedValue {
Absent(absentSpec: String),
Present {
spec: String,
value: f64
}
}
```
-- I mean, the spec should be the spec and stored in the same place whether it corresponds to an absent or a present SpecifiedValue. So if you think that's likely where we're headed, we should redesign this now, shouldn't we? Thoughts?
Vectornaut
commented
To me, it seems perfectly natural to have this: An absent the string I thought the main point of the To me, it seems perfectly natural to have this:
```rust
pub enum SpecifiedValue {
Absent(String),
Present {
spec: String,
value: f64
}
}
```
An absent `SpecifiedValue` has only a specification. A present `SpecifiedValue` has both a specification and a value. The specification is always stored in the same place. When I write something like
```
if let Ok(spec_val) = SpecifiedValue::try_from(my_spec) {
print!("{}", spec_val.spec());
}
```
the string `my_spec` is stored in `spec_val` and then returned by `spec_val.spec()`, regardless of whether `spec_val` is `Absent` or `Present`.
I thought the main point of the `SpecifiedValue` type was to store a specification together with all the value data derived from it, making it easy to ensure that the specification and the data always correspond. It's harder to do that if a `SpecifiedValue` doesn't carry its specification along with it.
glen
commented
Wow, I really don't understand why this detail of our design/implementation continues to be such a sticking point for us. It seems like it really should be simple:
We totally agree on this. So what is the best, clearest, simplest, easiest-to-use design of the SpecifiedValue type that embodies this principle? Feel free to start again from scratch if need be, I want to get this right.
We agree on this, too. Of course a SpecifiedValue should carry its specification with it -- it should be its primary data.
But to me this is patently not true in your corrected version of this potential way of implementing a SpecifiedValue with multiple ways of being Absent (sorry, I forgot tuple fields can't be named). In the Absent case, it's the 0th tuple entry of the payload. In the Present case, it's the Since SpecifiedValue is moving to a separate file anyway, should it be entirely opaque data-wise and have a method-only interface (and not be matchable upon, for example, in case at some point the best implementation isn't an enum)? The methods would I guess be something like is_present() and spec() and value(), with the latter I suppose returning As always, looking forward to your thoughts. And I do truly apologize (and am puzzled) that something seemingly so simple is being so recalcitrant of a mutually satisfying design/implementation. Wow, I really don't understand why this detail of our design/implementation continues to be such a sticking point for us. It seems like it really should be simple:
> I thought the main point of the SpecifiedValue type was to store a specification together with all the value data derived from it, making it easy to ensure that the specification and the data always correspond.
We totally agree on this. So what is the best, clearest, simplest, easiest-to-use design of the SpecifiedValue type that embodies this principle? Feel free to start again from scratch if need be, I want to get this right.
> It's harder to do that if a SpecifiedValue doesn't carry its specification along with it.
We agree on this, too. Of course a SpecifiedValue should carry its specification with it -- it should be its _primary_ data.
> The specification is always stored in the same place.
But to me this is patently not true in your corrected version of this potential way of implementing a SpecifiedValue with multiple ways of being Absent (sorry, I forgot tuple fields can't be named). In the Absent case, it's the 0th tuple entry of the payload. In the Present case, it's the `spec` field of the payload. Two different places for the "same" string -- to my eyes, very disorienting.
Since SpecifiedValue is moving to a separate file anyway, should it be entirely opaque data-wise and have a method-only interface (and _not_ be matchable upon, for example, in case at some point the best implementation isn't an enum)? The methods would I guess be something like is_present() and spec() and value(), with the latter I suppose returning `NaN` in case is_present() is false? Or perhaps value() panics if you call it when is_present() is false? That design change would immediately make the following conversation, about whether to have an is_present() method or use matching, moot and resolvable, because there would only be the methods.
As always, looking forward to your thoughts. And I do truly apologize (and am puzzled) that something seemingly so simple is being so recalcitrant of a mutually satisfying design/implementation.
Vectornaut
commented
If you find it less disorienting, we could do this: Now both variants have a field called To access the specification of a general The answers to this StackExchange question discuss various approaches to creating data types that have multiple variants with common fields.
> In the Absent case, it's the 0th tuple entry of the payload. In the Present case, it's the spec field of the payload.
If you find it less disorienting, we could do this:
```rust
pub enum SpecifiedValue {
Absent {
spec: String
},
Present {
spec: String,
value: f64
}
}
```
Now both variants have a field called `spec`, and the `Present` variant also has a field called `value`. This would make `match` arms that handle `Absent` and `Present` values look more similar.
To access the specification of a general `SpecifiedValue`, we'd still need to call the `spec()` method, which does the matching internally. As far as I know, different variants of an enum are always handled separately in the end. We can hide the separate handling within methods of the enum, but we can't get rid of it.
The answers to [this StackExchange question](https://stackoverflow.com/questions/49186751/sharing-a-common-value-in-all-enum-values) discuss various approaches to creating data types that have multiple variants with common fields.
- The current Proposal 1b approach, which uses the `spec()` method to access the specification field across all variants, is similar in spirit to [this answer](https://stackoverflow.com/a/77559109).
- When I was implementing Proposal 1a, I tried implementing the `Deref` trait for `OptionalSpecifiedValue`, kind of like in [this answer](https://stackoverflow.com/a/67467313). I switched to Proposal 1b before getting the trait working, though.
glen
commented
Hmm; it would seem to me that something like would be much closer to the spirit of that answer. There is always a spec, that's the primary data, and there may be a value, if the spec is a "Present" spec. Then we just need to make it clear what is the primary location/means of testing whether a SpecifiedValue is "present". I realize this may be full-circle... * Your thoughts on just a method-only interface with an opaque underlying datatype that we can iterate on if we see fit without changing any client code? Maybe our real sticking point is that the proposals so far are exposing too much of the implementation? Given this enduring debate, I am warming to such a way of ending it: I think we really _ought_ to be able to agree on what such an interface would be. Then the data layout of the underlying implementation is _much_ less critical -- basically anything reasonable that supports the interface.
> The current Proposal 1b approach, which uses the spec() method to access the specification field across all variants, is similar in spirit to this answer.
Hmm; it would seem to me that something like
```
struct SpecifiedValue {
spec: String,
value: Option<f64>
}
```
would be much closer to the spirit of that answer. There is always a spec, that's the primary data, and there may be a value, if the spec is a "Present" spec. Then we just need to make it clear what is **the** primary location/means of testing whether a SpecifiedValue is "present". I realize this may be full-circle...
Vectornaut
commented
At this point, I can't predict whether that would make things cleaner or messier. I'd want
I agree that this—let's call it Proposal 1c—would be close to the original Proposal 1. The big differences I see would be:
By the way, I think it's likely that we'll revisit the specified value data type when we generalize from decimal numbers to more generalized expressions. At that point, we'll probably be using specified values more, so we'll have a better idea of what we want from them. > Your thoughts on just a method-only interface with an opaque underlying datatype that we can iterate on if we see fit without changing any client code?
At this point, I can't predict whether that would make things cleaner or messier. I'd want `value`, and other methods that fetch derived data, to fail gracefully when the set point is absent—for example, by having an `Option` return type and returning `None` when the set point is absent. This might make it annoying to write client code that uses several pieces of derived data at once, but we don't have that problem yet, since there's only one piece of derived data.
> Hmm; it would seem to me that something like
>
> ```
> struct SpecifiedValue {
> spec: String,
> value: Option<f64>
> }
> ```
>
> would be much closer to the spirit of that answer. […] I realize this may be full-circle...
I agree that this—let's call it Proposal 1c—would be close to the original Proposal 1. The big differences I see would be:
- All the specified value data would be encapsulated in the `SpecifiedValue` type, instead of being loose in the regulator.
- When we only want to know whether the value is present, we'd call a method, which might internally check whether an arbitrary piece of derived data is `None` or `Some(_)`.
By the way, I think it's likely that we'll revisit the specified value data type when we generalize from decimal numbers to more generalized expressions. At that point, we'll probably be using specified values more, so we'll have a better idea of what we want from them.
Vectornaut
commented
I've switched to Proposal 1c (
I've switched to Proposal 1c (84bfdef), which we adopted during today's meeting. The `SpecifiedValue` structure is read-only, courtesy of the [readonly](https://docs.rs/readonly/latest/readonly/) crate, so nothing you do with it outside the `specified` module should be able to break the consistency between its fields. I've confirmed, for example, that outside the `specified` module:
- You can't construct a `SpecifiedValue` by manually initializing its fields.
- The fields of a `SpecifiedValue` can't be assigned to or borrowed as mutable.
glen
commented
Looks good. Let me just see if I now understand: currently in the code the only way to produce a SpecifiedValue is via the try_from operation on a string; and if one wants to change the set point of a regulator, it must be done by creating a fresh SpecifiedValue and replacing the prior set point with the fresh one (not by modifying the SpecifiedValue that's in place). Please let me know either way if I've got that all straight. Thanks. Looks good. Let me just see if I now understand: currently in the code the _only_ way to produce a SpecifiedValue is via the try_from operation on a string; and if one wants to change the set point of a regulator, it must be done by creating a fresh SpecifiedValue and replacing the prior set point with the fresh one (not by modifying the SpecifiedValue that's in place). Please let me know either way if I've got that all straight. Thanks.
Vectornaut
commented
There's one more way: the
Yup! > currently in the code the only way to produce a SpecifiedValue is via the try_from operation on a string;
There's one more way: the `from_empty_spec` method is public too.
> if one wants to change the set point of a regulator, it must be done by creating a fresh SpecifiedValue and replacing the prior set point with the fresh one (not by modifying the SpecifiedValue that's in place).
Yup!
|
||||
}
|
||||
|
||||
// the velocity is expressed in uniform coordinates
|
||||
pub struct ElementMotion<'a> {
|
||||
pub key: ElementKey,
|
||||
|
|
@ -236,7 +250,7 @@ impl Assembly {
|
|||
let mut gram_to_be = PartialMatrix::new();
|
||||
self.observables.with_untracked(|obsls| {
|
||||
for (_, obs) in obsls {
|
||||
if obs.role.with_untracked(|role| matches!(role, ObservableRole::Constrain)) {
|
||||
if obs.role_is_valid_constraint_untracked() {
|
||||
let subjects = obs.subjects;
|
||||
let row = elts[subjects.0].column_index.unwrap();
|
||||
let col = elts[subjects.1].column_index.unwrap();
|
||||
|
|
|
|||
|
|
@ -31,14 +31,14 @@ fn ObservableInput(observable: Observable) -> View {
|
|||
let target: HtmlInputElement = event.target().unwrap().unchecked_into();
|
||||
let value = target.value();
|
||||
if value.is_empty() {
|
||||
observable.role.set(Measure);
|
||||
observable.role.set(Measurement);
|
||||
} else {
|
||||
match target.value().parse::<f64>() {
|
||||
Ok(desired) => batch(|| {
|
||||
observable.desired.set(desired);
|
||||
observable.role.set(Constrain);
|
||||
observable.role.set(Constraint(true));
|
||||
}),
|
||||
Err(_) => observable.role.set(Invalid)
|
||||
Err(_) => observable.role.set(Constraint(false))
|
||||
|
glen marked this conversation as resolved
glen
commented
Please could you explain to me the three If I am understanding Rust correctly, the But it seems my suggestions in the previous paragraph can't possibly be correct, because And finally, the third Thanks in advance for illuminating these things for me. Please could you explain to me the three `move`s in the three closures in this `view!` macro, the one that specifies the class of the input element, the one that specifies its change handler, and the one that specifies its keydown handler?
If I am understanding Rust correctly, the `move` annotation specifies that the closure should take ownership of any free variables that appear therein. So that would mean that the function that computes the class of the input takes ownership of the regulator. Why is that desired? Is it because the input element may not actually be realized until after the RegulatorInput function returns, and the ownership transfer extends the lifetime of the regulator until the closure has a chance to run and grab the info it needs to set the class (or perhaps actually, it has to persist that regulator indefinitely and then it can react to all future changes to the regulator)? The class-calculating closure of the input element seems like a slightly odd place for the long-term ownership of the regulator to reside, but maybe it doesn't really matter what owns it, as long as it persists indefinitely?
But it seems my suggestions in the previous paragraph can't possibly be correct, because `regulator` also occurs free in the closure passed as `on:change`, but the ownership of one entity can't exist in two places, if I am understanding correctly. So I guess please just explain to me what's going on here.
And finally, the third `move` seems the most confusing. For the life of me, I can't see _any_ free variables in the closure passed as `on:keydown`, so there doesn't seem to be anything _to_ move, and so the `move` shouldn't be there.
Thanks in advance for illuminating these things for me.
glen
commented
Replying to your latest post here, to keep it in this resolvable conversation. Ah, the fact that Signal has Copy semantics makes this code much clearer. I didn't know that, and it seems a weird quirk of Rust to me that with ownership such a crucial concept to Rust, it is not clear whether copy or ownership transfer is in effect and I don't see any way you can know without consulting the source/docs for Signal. Anyhow, I now get the first two Replying to your latest post here, to keep it in this resolvable conversation. Ah, the fact that Signal has Copy semantics makes this code much clearer. I didn't know that, and it seems a weird quirk of Rust to me that with ownership such a crucial concept to Rust, it is not clear whether copy or ownership transfer is in effect and I don't see any way you can know without consulting the source/docs for Signal.
Anyhow, I now get the first two `move`s. And now I see that `reset_value` is just a local closure, and that's what's being `move`d by the third `move` (for the keydown event). So that's good. Resolving.
|
||||
};
|
||||
}
|
||||
}
|
||||
|
|
@ -60,9 +60,9 @@ fn ObservableOutlineItem(observable_key: ObservableKey, element_key: ElementKey)
|
|||
let other_subject_label = assembly.elements.with(|elts| elts[other_subject].label.clone());
|
||||
let class = observable.role.map(
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
This looks like the code is checking the OK-ness of the try_from twice. Is there a concise clear way to bind a variable to the SpecifiedValue payload in the OK return case and set P.S. If the code here switched to something like this suggestion, then as this is the only instance of So I guess I am specifically suggesting replacing this block with -- presuming the syntax is OK -- and just removing that This looks like the code is checking the OK-ness of the try_from twice. Is there a concise clear way to bind a variable to the SpecifiedValue payload in the OK return case and set `valid` to true and call the as-yet-unwritten regulator.set with that specified value, in that case, and in the alternative (Error) case, just set `valid` to false and do nothing to the regulator? That organization would seem to read more crisply, as long as it isn't too cumbersome to write... And in fact, if I recall, isn't that sort of thing what `if let OK(blah) = try_from { ... } else {valid.set(false)}` is for?
P.S. If the code here switched to something like this suggestion, then as this is the only instance of `set_if_ok`, you could remove that method in favor of a simpler `set` method, or in fact maybe just a direct call of `regulator.set_point.set(...)`, eliminating the entire `impl Regulator` block at lines 127-135 of assembly.rs, which seems like a win.
So I guess I am specifically suggesting replacing this block with
```
if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) {
valid.set(true)
regulator.set_point.set(spec)
} else { valid.set(false) }
```
-- presuming the syntax is OK -- and just removing that `impl Regulator` block in assembly.rs altogether.
Vectornaut
commented
That sounds great to me. I'd reorganize the block to avoid the repetition of In the > So I guess I am specifically suggesting replacing this block with
>
> ```rust
> if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) {
> valid.set(true)
> regulator.set_point.set(spec)
> } else { valid.set(false) }
That sounds great to me. I'd reorganize the block to avoid the repetition of `valid.set`. Depending on whether you prefer `if let` or `match`, we could do either of these:
```rust
valid.set(
if let Ok(set_pt) = SpecifiedValue::try_from(value.get_clone_untracked()) {
regulator.set_point.set(set_pt);
true
} else { false }
)
```
```rust
valid.set(
match SpecifiedValue::try_from(value.get_clone_untracked()) {
Ok(set_pt) => {
regulator.set_point.set(set_pt);
true
}
Err(_) => false
}
)
```
In the `match` version, the argument of `valid.set` is just an inlined version of the old `Regulator::try_set` method. If we want to go this route, and you don't have a strong preference for inlining, I'd recommend just reverting commit 894931a, which replaced `Regulator::try_set` with `Regulator::set_if_ok`. We can also rewrite `try_set` to use `if let` instead of `match`, of course.
|
||||
|role| match role {
|
||||
Measure => "observable",
|
||||
Constrain => "observable constrained",
|
||||
Invalid => "observable invalid"
|
||||
Measurement => "observable",
|
||||
Constraint(true) => "observable valid-constraint",
|
||||
Constraint(false) => "observable invalid-constraint"
|
||||
}
|
||||
);
|
||||
view! {
|
||||
|
|
|
|||
Tiny writing nit: this is a "garden path" phrasing -- I read "format discussed above" and started looking for the format specification earlier in the file, since I was pretty sure I hadn't seen one. Maybe reword just a bit to avoid that trap I fell into?
Nice catch! I've changed "above" to "at" to resolve the ambiguity (
7cbd926). Does that read all right to you?I agree unambiguous now. And hopefully this will change to an explicit anchor link when we get the doc system set up. So resolving.