StudioInfinity/dyna3
2
0
Fork 1
Code Issues 44 Pull requests Projects 2 Releases 2 Wiki Activity Actions

Generalize constraints to observables #48

Merged
glen merged 25 commits from Vectornaut/dyna3:observables_on_main into main 2025-03-10 23:43:25 +00:00
Conversation 69 Commits 25 Files changed 7 +97 -53
2 changed files with 97 additions and 53 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 6c31a25822 - Show all commits

127
app-proto/src/assembly.rs
View file

@ -1,7 +1,11 @@
use nalgebra::{DMatrix, DVector, DVectorView, Vector3};
use rustc_hash::FxHashMap;
use slab::Slab;
use std::{collections::BTreeSet, sync::atomic::{AtomicU64, Ordering}};
use std::{
collections::BTreeSet,
num::ParseFloatError,
sync::atomic::{AtomicU64, Ordering}
};
use sycamore::prelude::*;
use web_sys::{console, wasm_bindgen::JsValue}; /* DEBUG */
@ -111,41 +115,75 @@ impl Element {
}
}
// `set_point_spec` is always a valid specification of `set_point`
// ┌────────────┬─────────────────────────────────────────────────────┐
// │`set_point` │ `set_point_spec` │
// ┝━━━━━━━━━━━━┿━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┥
// │`Some(x)` │ a string that parses to the floating-point value `x`│
// ├────────────┼─────────────────────────────────────────────────────┤
// │`None` │ the empty string │
// └────────────┴─────────────────────────────────────────────────────┘
// to construct a `SpecifiedValue` that might be `Present`, use the associated
// function `try_from`. this ensures that `spec` is always a valid specification
// of `value` according to the format discussed above the implementation of
glen marked this conversation as resolved Outdated
glen commented 2025-02-25 23:49:20 +00:00
Outdated
Review
Copy link

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?

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?
Vectornaut commented 2025-02-27 08:34:08 +00:00
Outdated
Review
Copy link

Nice catch! I've changed "above" to "at" to resolve the ambiguity (7cbd926). Does that read all right to you?

Nice catch! I've changed "above" to "at" to resolve the ambiguity (7cbd926). Does that read all right to you?
glen commented 2025-03-01 01:32:09 +00:00
Outdated
Review
Copy link

I agree unambiguous now. And hopefully this will change to an explicit anchor link when we get the doc system set up. So resolving.

I agree unambiguous now. And hopefully this will change to an explicit anchor link when we get the doc system set up. So resolving.
// `TryFrom<String>`
pub enum SpecifiedValue {
Absent,
Present {
spec: String,
value: f64
}
}
use SpecifiedValue::*;
impl SpecifiedValue {
// get the specification for this value. the associated function `try_from`
// is essentially a left inverse of this method:
//
// SpecifiedValue::try_from(x.spec()) == Ok(x)
//
pub fn spec(&self) -> String {
match self {
Absent => String::new(),
glen marked this conversation as resolved Outdated
glen commented 2025-02-26 00:01:30 +00:00
Outdated
Review
Copy link

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.

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 2025-02-27 08:34:49 +00:00
Outdated
Review
Copy link

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.

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 2025-03-01 01:36:18 +00:00
Outdated
Review
Copy link

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?

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 2025-03-01 02:26:45 +00:00
Outdated
Review
Copy link

To me, it seems perfectly natural to have this:

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.

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 2025-03-01 02:46:26 +00:00
Outdated
Review
Copy link

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.

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 2025-03-01 03:09:57 +00:00
Outdated
Review
Copy link

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:

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 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.
  • When I was implementing Proposal 1a, I tried implementing the Deref trait for OptionalSpecifiedValue, kind of like in this answer. I switched to Proposal 1b before getting the trait working, though.
> 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 2025-03-01 03:34:51 +00:00
Outdated
Review
Copy link
  • 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...

* 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 2025-03-03 23:41:23 +00:00
Outdated
Review
Copy link

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.

> 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 2025-03-04 07:36:34 +00:00
Outdated
Review
Copy link

I've switched to Proposal 1c (84bfdef), which we adopted during today's meeting. The SpecifiedValue structure is read-only, courtesy of the 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.
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 2025-03-04 15:42:32 +00:00
Outdated
Review
Copy link

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 2025-03-10 19:50:35 +00:00
Outdated
Review
Copy link

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!

> 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!
Present { spec, .. } => spec.clone()
}
}
fn is_present(&self) -> bool {
match self {
glen marked this conversation as resolved Outdated
glen commented 2025-02-26 00:10:26 +00:00
Outdated
Review
Copy link

Is it worthwhile/idiomatic to provide such a predicate when clients of SpecifiedValue could just (a) match themselves, (b) use if let, (c) we could just derive PartialEq and let clients test sv != SpecifiedValue::Absent, or (d) use the matches!() macro, any one of which might be totally readable and just as easy? In other words, is this method reinventing the wheel? Not saying it's necessarily extraneous, just wanted to raise the issue in case we can basically save some code by using some preexisting standard idiom.

Is it worthwhile/idiomatic to provide such a predicate when clients of SpecifiedValue could just (a) match themselves, (b) use if let, (c) we could just derive PartialEq and let clients test sv != SpecifiedValue::Absent, or (d) use the matches!() macro, any one of which might be totally readable and just as easy? In other words, is this method reinventing the wheel? Not saying it's necessarily extraneous, just wanted to raise the issue in case we can basically save some code by using some preexisting standard idiom.
Vectornaut commented 2025-02-27 08:17:18 +00:00
Outdated
Review
Copy link

Several enums from the standard library have similar methods.

In the tracking issue for those Cow methods, there's a hearty debate about whether such methods are a good idea. Pattern-matching has improved since Option and Result were introduced, and it may eventually improve enough to make variant-checking methods obsolete. For now, though, I personally think that these methods pay for themselves in readability.

Several enums from the standard library have similar methods. - `Option`: [`is_some`](https://doc.rust-lang.org/std/option/enum.Option.html#method.is_some) / [`is_none`](https://doc.rust-lang.org/std/option/enum.Option.html#method.is_none) - `Result`: [`is_ok`](https://doc.rust-lang.org/std/result/enum.Result.html#method.is_ok) / [`is_err`](https://doc.rust-lang.org/std/result/enum.Result.html#method.is_err) - `Cow`: [`is_borrowed`](https://doc.rust-lang.org/std/borrow/enum.Cow.html#method.is_borrowed) / [`is_owned`](https://doc.rust-lang.org/std/borrow/enum.Cow.html#method.is_owned) In the [tracking issue](https://github.com/rust-lang/rust/issues/65143) for those `Cow` methods, there's a hearty debate about whether such methods are a good idea. Pattern-matching has [improved](https://github.com/rust-lang/rust/issues/65143#issuecomment-543955294) since `Option` and `Result` were introduced, and it may eventually [improve enough](https://github.com/rust-lang/rfcs/pull/3573) to make variant-checking methods obsolete. For now, though, I personally think that these methods pay for themselves in readability.
glen commented 2025-03-01 01:55:33 +00:00
Outdated
Review
Copy link

So sounds like you are in the camp that would vote for adoption of RFC 3573? It appears mired in non-converging conversation, and it's not clear to me that the Rust leadership has any mechanism for creating a predictable period in which to expect action on a given RFC. If indeed you agree that it would be a good feature, then I think we should (a) commit to adopting the is syntax in Husht, and (b) in the meantime not have this method (to avoid repeating was is now seen as a wart in the standard library proliferation of these methods) and instead use one of the two forms in the RFC discussion that are said to map one-to-one to is, either an if let or a matches!() so that as Husht matures, the conversion will at some point rewrite this to the is syntax we both like better. Thoughts?

So sounds like you are in the camp that would vote for adoption of RFC 3573? It appears mired in non-converging conversation, and it's not clear to me that the Rust leadership has any mechanism for creating a predictable period in which to expect action on a given RFC. If indeed you agree that it would be a good feature, then I think we should (a) commit to adopting the `is` syntax in Husht, and (b) in the meantime not have this method (to avoid repeating was is now seen as a wart in the standard library proliferation of these methods) and instead use one of the two forms in the RFC discussion that are said to map one-to-one to is, either an `if let` or a `matches!()` so that as Husht matures, the conversion will at some point rewrite this to the `is` syntax we both like better. Thoughts?
Vectornaut commented 2025-03-01 02:37:13 +00:00
Outdated
Review
Copy link

I think it's best to keep Husht as close as possible to Rust in terms of features, for two reasons.

  • If a proposed Rust feature gets adopted by Husht but not by Rust, there's a danger that Rust will eventually adopt a conflicting version of the same feature.
  • I think that the more an "associated language" diverges from the base language, the harder it is for people to transfer skills in either direction. For example, Processing diverges substantially from Java. That made it harder for me to transfer my Java skills to Processing, and I think it also makes it hard for people to transfer their Processing skills to Java.

[…] to avoid repeating was is now seen as a wart in the standard library proliferation of these methods […]

I don't get the impression that it's seen as a wart. Lots of people are commenting on that tracking issue to say that they appreciate these methods and prefer them over existing alternatives.

I think it's best to keep Husht as close as possible to Rust in terms of features, for two reasons. - If a proposed Rust feature gets adopted by Husht but not by Rust, there's a danger that Rust will eventually adopt a conflicting version of the same feature. - I think that the more an "associated language" diverges from the base language, the harder it is for people to transfer skills in either direction. For example, Processing diverges substantially from Java. That made it harder for me to transfer my Java skills to Processing, and I think it also makes it hard for people to transfer their Processing skills to Java. > […] to avoid repeating was is now seen as a wart in the standard library proliferation of these methods […] I don't get the impression that it's seen as a wart. Lots of people are commenting on that [tracking issue](https://github.com/rust-lang/rust/issues/65143) to say that they appreciate these methods and prefer them over existing alternatives.
glen commented 2025-03-01 03:05:47 +00:00
Outdated
Review
Copy link

prefer them over existing alternatives [emphasis mine]

Exactly: the gist of the majority of such comments, in my relatively diligent reading of the thread (I mean, I did not read but a tiny fraction of the hundreds of hidden comments, but I did read all unhidden ones) is that the need for such methods exists because there is not a clean general is operator, and if there were, they would prefer that over the proliferation of is_borrowed etc.

> prefer them over _existing_ alternatives [emphasis mine] Exactly: the gist of the majority of such comments, in my relatively diligent reading of the thread (I mean, I did not read but a tiny fraction of the hundreds of hidden comments, but I did read all unhidden ones) is that the need for such methods exists _because_ there is not a clean general `is` operator, and if there were, they would prefer that over the proliferation of `is_borrowed` etc.
glen commented 2025-03-01 03:18:27 +00:00
Outdated
Review
Copy link

@Vectornaut wrote in glen/dyna3#48 (comment):

I think it's best to keep Husht as close as possible to Rust in terms of features, for two reasons.

This is not the place to debate Husht, we have a whole repo for that, but my motivation for it is to provide a concise, readable, convenient Rust. I think I've made it clear that Civet:TypeScript::Husht:Rust is roughly my vision. One of the more minor alterations Civet does, and that I would expect Husht to do, is early adoption of JavaScript/TypeScript feature proposals. And so what if the feature ends up officially Rust-adopted slightly differently than we implemented it in Husht? We drop it from Husht and refactor our code. These things are just not big deals, in my experience. I was an early adopter of Civet and so went through several such refactors as Civet evolved. It was not particularly painful, and the comfort of coding in a concise, expressive, DRY language was well worth the effort.

In short, Husht is for my understandability and our productivity. I am not so worried about transference from Rust. CoffeeScript (sort of the same thing 4-8 years ago) became famous as a laboratory for JavaScript features, many of which were adopted into future JavaScript versions. I don't really expect that for Husht because Rust seems to attract folks on the somewhat pedantic/rigid side, who really seem to want a style monoculture, but it's always a remote possibility that if we really flesh out Husht, it could attract some following of people that want a style-variation-tolerant, concise language with the safety/webassemblable/whatever features of Rust. But I don't really care whether it does or not: For this project long term I need a comfortable language to read and develop in. It's even possible that some of our debates that we're getting hung up on are driven in part by Rust's verbose, rigid syntax.

@Vectornaut wrote in https://code.studioinfinity.org/glen/dyna3/pulls/48#issuecomment-2271: > I think it's best to keep Husht as close as possible to Rust in terms of features, for two reasons. This is not the place to debate Husht, we have a whole repo for that, but my motivation for it is to provide a concise, readable, convenient Rust. I think I've made it clear that Civet:TypeScript::Husht:Rust is roughly my vision. One of the more minor alterations Civet does, and that I would expect Husht to do, is early adoption of JavaScript/TypeScript feature proposals. And so what if the feature ends up officially Rust-adopted slightly differently than we implemented it in Husht? We drop it from Husht and refactor our code. These things are just not big deals, in my experience. I was an early adopter of Civet and so went through several such refactors as Civet evolved. It was not particularly painful, and the comfort of coding in a concise, expressive, DRY language was well worth the effort. In short, Husht is for my understandability and our productivity. I am not so worried about transference from Rust. CoffeeScript (sort of the same thing 4-8 years ago) became famous as a laboratory for JavaScript features, _many_ of which were adopted into future JavaScript versions. I don't really expect that for Husht because Rust seems to attract folks on the somewhat pedantic/rigid side, who _really_ seem to want a style monoculture, but it's always a remote possibility that if we really flesh out Husht, it could attract some following of people that want a style-variation-tolerant, concise language with the safety/webassemblable/whatever features of Rust. But I don't really care whether it does or not: For this project long term I need a comfortable language to read and develop in. It's even possible that some of our debates that we're getting hung up on are driven in part by Rust's verbose, rigid syntax.
Vectornaut commented 2025-03-01 03:29:25 +00:00
Outdated
Review
Copy link

This is not the place to debate Husht […]

Agreed. I think you get the tradeoff at this point, so let me know whether you want SpecifiedValue to have an is_present method in this pull request.

> This is not the place to debate Husht […] Agreed. I think you get the tradeoff at this point, so let me know whether you want `SpecifiedValue` to have an `is_present` method in this pull request.
glen commented 2025-03-01 06:31:04 +00:00
Outdated
Review
Copy link

Well to me Occam says no "is_present," so that's my preference, so please go with that if you feel only a mild preference (or less) for having it. Since you're doing essentially all of the implementation at the moment, if you have more than a mild preference for having it, leave it in. Thanks for discussing the pros and cons.

Well to me Occam says no "is_present," so that's my preference, so please go with that if you feel only a mild preference (or less) for having it. Since you're doing essentially all of the implementation at the moment, if you have more than a mild preference for having it, leave it in. Thanks for discussing the pros and cons.
Vectornaut commented 2025-03-03 21:18:21 +00:00
Outdated
Review
Copy link

I've removed if_present, replacing it with matches!(/* ... */, Present { .. }) where it was used (c58fed0).

I've removed `if_present`, replacing it with `matches!(/* ... */, Present { .. })` where it was used (c58fed0).
Absent => false,
Present { .. } => true
}
}
}
// we can try to turn a specification string into a `SpecifiedValue`. if the
// specification is empty, the `SpecifiedValue` is `Absent`. if the
// specification parses to a floating-point value `x`, the `SpecifiedValue` is
// `Present`, with a `value` of `x`, and the specification is stored in `spec`.
// these are the only valid specifications; any other produces an error
glen marked this conversation as resolved Outdated
glen commented 2025-02-25 23:50:55 +00:00
Outdated
Review
Copy link

Maybe add "currently" -- "These are currently the only valid..." -- since we are definitely expecting to add other specifications (if we weren't, we would likely not go to this trouble).

Maybe add "currently" -- "These are currently the only valid..." -- since we are definitely expecting to add other specifications (if we weren't, we would likely not go to this trouble).
Vectornaut commented 2025-02-27 08:35:27 +00:00
Outdated
Review
Copy link

Done (7cbd926).

Done (7cbd926).
impl TryFrom<String> for SpecifiedValue {
type Error = ParseFloatError;
fn try_from(spec: String) -> Result<Self, Self::Error> {
if spec.is_empty() {
Ok(Absent)
} else {
spec.parse::<f64>().map(
|value| Present { spec: spec, value: value }
)
}
}
}
#[derive(Clone, Copy)]
pub struct Regulator {
pub subjects: (ElementKey, ElementKey),
pub measurement: ReadSignal<f64>,
pub set_point: ReadSignal<Option<f64>>,
set_point_writable: Signal<Option<f64>>,
set_point_spec: Signal<String>
pub set_point: Signal<SpecifiedValue>
}
impl Regulator {
pub fn get_set_point_spec_clone(&self) -> String {
self.set_point_spec.get_clone()
}
pub fn get_set_point_spec_clone_untracked(&self) -> String {
self.set_point_spec.get_clone_untracked()
}
pub fn try_specify_set_point(&self, spec: String) -> bool {
match spec.parse::<f64>() {
Err(_) if !spec.is_empty() => false,
set_pt => {
self.set_point_writable.set(set_pt.ok());
self.set_point_spec.set(spec);
pub fn try_set(&self, set_pt_spec: String) -> bool {
glen marked this conversation as resolved Outdated
glen commented 2025-02-26 00:13:27 +00:00
Outdated
Review
Copy link

Why the difference in the interface between SpecifiedValue::try_from and Regulator::try_set? I.e., it seems to me that they could both return a Result, or could both return a boolean success flag...

Why the difference in the interface between SpecifiedValue::try_from and Regulator::try_set? I.e., it seems to me that they could both return a Result, or could both return a boolean success flag...
Vectornaut commented 2025-02-26 20:28:57 +00:00
Outdated
Review
Copy link

We could indeed modify try_set so that it returns a clone of the Result<SpecifiedValue> computed from the given specification. This might be useful if the caller wants a detailed error report when the try_set fails. Right now, though, the code that calls try_set only wants to know whether the attempt succeeded.

The associated function try_from is a constructor, so it can't just return a success flag. Would revising the comment on the TryFrom implementation help communicate this? Readers already familiar with the TryFrom trait would know this already, but making documentation broadly accessible is often worthwhile.

We could indeed modify `try_set` so that it returns a clone of the `Result<SpecifiedValue>` computed from the given specification. This might be useful if the caller wants a detailed error report when the `try_set` fails. Right now, though, the code that calls `try_set` only wants to know whether the attempt succeeded. The associated function `try_from` is a constructor, so it can't just return a success flag. Would revising the comment on the `TryFrom` implementation help communicate this? Readers already familiar with the [`TryFrom`](https://doc.rust-lang.org/std/convert/trait.TryFrom.html) trait would know this already, but making documentation broadly accessible is often worthwhile.
glen commented 2025-02-27 03:58:03 +00:00
Outdated
Review
Copy link

OK, you're saying try_from generates a SpecifiedValue, so if it succeeds, we definitely need to provide that entity. That makes sense.

What seems a bit confusing is a different interface on what's almost the identical operation. Why would try_set have to return a clone? Doesn't Rust allow a read-only alias to a struct? Or doesn't the Regulator already have a ReadSignal for its SpecifiedValue? Couldn't a Result of that ReadSignal be provided in case of success or something like that? Presumably it's not hard to just check if what comes back is a Result rather than an Error, if that's all you care about?

OK, you're saying try_from generates a SpecifiedValue, so if it succeeds, we definitely need to provide that entity. That makes sense. What seems a bit confusing is a different interface on what's almost the identical operation. Why would try_set have to return a clone? Doesn't Rust allow a read-only alias to a struct? Or doesn't the Regulator already have a ReadSignal for its SpecifiedValue? Couldn't a Result of that ReadSignal be provided in case of success or something like that? Presumably it's not hard to just check if what comes back is a Result rather than an Error, if that's all you care about?
Vectornaut commented 2025-02-27 07:31:29 +00:00
Outdated
Review
Copy link

Or doesn't the Regulator already have a ReadSignal for its SpecifiedValue?

Yes: the set_point field is public. Thus, there's no need for try_set to return something that gives you access to the set point. If you have access to a regulator's try_set method, you also have access to its set_point field.

Why would try_set have to return a clone?

Here's the story as I vaguely understand it, based on my shaky knowledge of ownership in Rust. When the specification is valid, try_from returns a result of the form Ok(set_pt). We then call self.set_point.set(set_pt), which moves set_pt into the signal self.set_point. That means we can't return set_pt. I don't see a straightforward way to extract a reference to the value stored in a signal. I can use Signal::with to apply a closure to a reference to the stored value, but if I try to return that reference, the compiler can't infer an appropriate lifetime for it. Someone who understands lifetimes better might be able to make this work, but nothing I've tried has worked.

If your main goal is to return a detailed error report when try_set fails, we could have try_set return a Result<(), ParseFloatError>.

> Or doesn't the Regulator already have a ReadSignal for its SpecifiedValue? Yes: the `set_point` field is public. Thus, there's no need for `try_set` to return something that gives you access to the set point. If you have access to a regulator's `try_set` method, you also have access to its `set_point` field. > Why would try_set have to return a clone? Here's the story as I vaguely understand it, based on my shaky knowledge of ownership in Rust. When the specification is valid, `try_from` returns a result of the form `Ok(set_pt)`. We then call `self.set_point.set(set_pt)`, which *moves* `set_pt` into the signal `self.set_point`. That means we can't return `set_pt`. I don't see a straightforward way to extract a reference to the value stored in a signal. I can use `Signal::with` to apply a closure to a reference to the stored value, but if I try to return that reference, the compiler can't infer an appropriate lifetime for it. Someone who understands lifetimes better might be able to make this work, but nothing I've tried has worked. If your main goal is to return a detailed error report when `try_set` fails, we could have `try_set` return a `Result<(), ParseFloatError>`.
glen commented 2025-03-01 02:02:45 +00:00
Outdated
Review
Copy link

No, my goal is not to have two non-parallel interfaces for essentially the same operation. But you are right, currently the error info is simply lost when you use try_set rather than try_from. Which begs the question: if I already have a SpecifiedValue, is there a way to make it the set_point of a Regulator? Perhaps there should be; it seems as though it can't ever be wrong to make the set_point be any actual SpecifiedValue. And so maybe that would actually be the best solution to this nonparallelism, because there would only be one interface. To go from a possible string that might be a spec to changing the set_point of a Regulator, you first try_from it into a SpecifiedValue and then upon success set the set_point to that. Then you will at least receive the error info, which you can ignore if you like. Would such a refactoring actually be the cleanest interface? Looking forward to your thoughts.

No, my goal is not to have two non-parallel interfaces for essentially the same operation. But you are right, currently the error info is simply lost when you use try_set rather than try_from. Which begs the question: if I already have a SpecifiedValue, is there a way to make it the set_point of a Regulator? Perhaps there should be; it seems as though it can't ever be wrong to make the set_point be any actual SpecifiedValue. And so maybe that would actually be the best solution to this nonparallelism, because there would only be one interface. To go from a possible string that might be a spec to changing the set_point of a Regulator, you first try_from it into a SpecifiedValue and then upon success set the set_point to that. Then you will at least receive the error info, which you can ignore if you like. Would such a refactoring actually be the cleanest interface? Looking forward to your thoughts.
Vectornaut commented 2025-03-01 02:42:36 +00:00
Outdated
Review
Copy link

[…] if I already have a SpecifiedValue, is there a way to make it the set_point of a Regulator? Perhaps there should be […]

Yes, I think we should add a set method for this as soon as we have a reason to use it.

And so maybe that would actually be the best solution to this nonparallelism, because there would only be one interface.

Even if there were a set method, though, I would still want to keep try_set as a wrapper until we get to a point where we never have a reason to use it. I think that try_set is a concise way to express an operation that feels natural and self-contained.

> […] if I already have a SpecifiedValue, is there a way to make it the set_point of a Regulator? Perhaps there should be […] Yes, I think we should add a `set` method for this as soon as we have a reason to use it. > And so maybe that would actually be the best solution to this nonparallelism, because there would only be one interface. Even if there were a `set` method, though, I would still want to keep `try_set` as a wrapper until we get to a point where we never have a reason to use it. I think that `try_set` is a concise way to express an operation that feels natural and self-contained.
glen commented 2025-03-01 02:59:59 +00:00
Outdated
Review
Copy link

I am not yet fluent in Rust syntax, so I am sure the below are not correct, but why does something like

myReg.set(try_from('foobar'))

need an abbreviation

myReg.try_set('foobar')

Now a point might be that try_from doesn't give you a SpecifiedValue, it gives you a Result<SpecifiedValue, SomeKindaError>. But then wouldn't it be most natural just to overload set to also take such an entity, rather than create a new method? And then if you do want the error payload, you could first capture the result of try_from and match on it, and do whatever with the error if that's what matches? This factoring seems simpler and less redundant to me. I guess my point is, we are already setting Regulators, so we already have a reason for such a method; and we can already try to convert a string to a SpecifiedValue; and so we don't need a redundant combo of the two with a different interface, it seems to me...

I am not yet fluent in Rust syntax, so I am sure the below are not correct, but why does something like ``` myReg.set(try_from('foobar')) ``` need an abbreviation ``` myReg.try_set('foobar') ``` Now a point might be that try_from doesn't give you a SpecifiedValue, it gives you a `Result<SpecifiedValue, SomeKindaError>`. But then wouldn't it be most natural just to overload `set` to also take such an entity, rather than create a new method? And then if you do want the error payload, you could first capture the result of try_from and match on it, and do whatever with the error if that's what matches? This factoring seems simpler and less redundant to me. I guess my point is, we are _already_ `set`ting Regulators, so we already have a reason for such a method; and we can already try to convert a string to a SpecifiedValue; and so we don't need a redundant combo of the two with a different interface, it seems to me...
Vectornaut commented 2025-03-01 03:26:39 +00:00
Outdated
Review
Copy link

Now a point might be that try_from doesn't give you a SpecifiedValue, it gives you a Result<SpecifiedValue, SomeKindaError>.

Exactly.

But then wouldn't it be most natural just to overload set to also take such an entity, rather than create a new method?

As far as I know, Rust doesn't have function overloading, so the versions of set that take a SpecifiedValue and a Result<SpecifiedValue, ParseFloatError> would have to have different names. We could choose set and set_if_ok, for example. I could look through the standard library to find out what kind of naming might be idiomatic.

> Now a point might be that try_from doesn't give you a SpecifiedValue, it gives you a Result<SpecifiedValue, SomeKindaError>. Exactly. > But then wouldn't it be most natural just to overload set to also take such an entity, rather than create a new method? As far as I know, Rust doesn't have function overloading, so the versions of `set` that take a `SpecifiedValue` and a `Result<SpecifiedValue, ParseFloatError>` would have to have different names. We could choose `set` and `set_if_ok`, for example. I could look through the standard library to find out what kind of naming might be idiomatic.
glen commented 2025-03-01 06:18:28 +00:00
Outdated
Review
Copy link

Rust doesn't have function overloading

Wow that's kind of wild and seems to invite non-DRYness (repeating the type of the operand in the function name, in effect).

If this basic interface idea seems OK, then the names of the two flavors of set are up to you, and you can only implement the one you actually end up wanting to use if it's only one, but please do comment what you would plan to name the other one.

> Rust doesn't have function overloading Wow that's kind of wild and seems to invite non-DRYness (repeating the type of the operand in the function name, in effect). If this basic interface idea seems OK, then the names of the two flavors of set are up to you, and you can only implement the one you actually end up wanting to use if it's only one, but please do comment what you would plan to name the other one.
glen commented 2025-03-01 06:28:23 +00:00
Outdated
Review
Copy link

Rust doesn't have function overloading

Oh, this SO answer says the way to support myReg.set(aPlainSpecifiedValue) and myReg.set(aResultThatCouldBe) with the choice made at compile time based on the type of the argument is with a parametrized Trait, which is pretty much the same thing as supporting function overloading. So that would seem preferable to me to two different names; but if you strongly prefer to not use a trait and have two names in this case, and plan them out in this PR, I can live with that, too.

> Rust doesn't have function overloading Oh, [this SO answer](https://stackoverflow.com/a/56509699) says the way to support `myReg.set(aPlainSpecifiedValue)` and `myReg.set(aResultThatCouldBe)` with the choice made at compile time based on the type of the argument is with a parametrized Trait, which is pretty much the same thing as supporting function overloading. So that would seem preferable to me to two different names; but if you strongly prefer to not use a trait and have two names in this case, and plan them out in this PR, I can live with that, too.
Vectornaut commented 2025-03-01 09:26:33 +00:00
Outdated
Review
Copy link

The idea of using a parameterized trait is nice, but in this case I'd prefer to use methods named set and set_if_ok. The former always updates the set point, while the latter only updates the set point under a certain condition, and I like how the different names communicate that.

please do comment what you would plan to name the other one.

Just here, or in the code too?

The idea of using a parameterized trait is nice, but in this case I'd prefer to use methods named `set` and `set_if_ok`. The former always updates the set point, while the latter only updates the set point under a certain condition, and I like how the different names communicate that. > please do comment what you would plan to name the other one. Just here, or in the code too?
glen commented 2025-03-01 14:05:24 +00:00
Outdated
Review
Copy link

In the code, thanks.

In the code, thanks.
Vectornaut commented 2025-03-03 21:16:29 +00:00
Outdated
Review
Copy link

Done (894931a).

Done (894931a).
match SpecifiedValue::try_from(set_pt_spec) {
Ok(set_pt) => {
self.set_point.set(set_pt);
true
}
Err(_) => false,
}
}
}
@ -255,18 +293,15 @@ impl Assembly {
reps.0.dot(&(&*Q * reps.1))
}
);
let set_point_writable = create_signal(None);
let set_point = *set_point_writable;
let set_point = create_signal(Absent);
glen marked this conversation as resolved Outdated
glen commented 2025-02-26 00:29:53 +00:00
Outdated
Review
Copy link

Please illuminate me: Why do we not need to write SpecifiedValue::Absent here? I mean, I like the brevity, but I think I am unclear about when the bare variants are in scope...

Please illuminate me: Why do we not need to write `SpecifiedValue::Absent` here? I mean, I like the brevity, but I think I am unclear about when the bare variants are in scope...
Vectornaut commented 2025-02-26 20:36:56 +00:00
Outdated
Review
Copy link

The declaration

use SpecifiedValue::*;

puts all variants in scope. I currently have it just after the definition of SpecifiedValue, because it felt out of context elsewhere. For example, I usually expect use declarations at the top of the file to be external.

The declaration ``` use SpecifiedValue::*; ``` puts all variants in scope. I currently have it just after the definition of `SpecifiedValue`, because it felt out of context elsewhere. For example, I usually expect use declarations at the top of the file to be external.
glen commented 2025-02-27 03:44:05 +00:00
Outdated
Review
Copy link

Ah, the fact that use is sort of "lost" in the midst of the file suggests that SpecifiedValue should be in its own source file and imported here, so the relevant use is up where one would generally look. assembly.rs is getting pretty huge anyway... What are your thought about that factoring?

Ah, the fact that `use` is sort of "lost" in the midst of the file suggests that SpecifiedValue should be in its own source file and imported here, so the relevant `use` is up where one would generally look. assembly.rs is getting pretty huge anyway... What are your thought about that factoring?
Vectornaut commented 2025-02-27 08:23:18 +00:00
Outdated
Review
Copy link

Specified values and assemblies do seem like pretty independent concepts, so putting SpecifiedValue in its own module might be reasonable.

Specified values and assemblies do seem like pretty independent concepts, so putting `SpecifiedValue` in its own module might be reasonable.
glen commented 2025-03-01 02:07:42 +00:00
Outdated
Review
Copy link

Are you doing so in this PR? That's my (relatively mild) recommendation.

Are you doing so in this PR? That's my (relatively mild) recommendation.
Vectornaut commented 2025-03-01 02:09:08 +00:00
Outdated
Review
Copy link

Yes, I'll do this—I was just waiting for a go-ahead from you. I should've mentioned that in the last comment.

Yes, I'll do this—I was just waiting for a go-ahead from you. I should've mentioned that in the last comment.
👍 1
Vectornaut commented 2025-03-03 21:19:19 +00:00
Outdated
Review
Copy link

Done (309b088).

Done (309b088).
self.insert_regulator(Regulator {
subjects: subjects,
measurement: measurement,
set_point: set_point,
set_point_writable: set_point_writable,
set_point_spec: create_signal(String::new())
set_point: set_point
});
/* DEBUG */
// print updated regulator list
// print an updated list of regulators
console::log_1(&JsValue::from("Regulators:"));
glen marked this conversation as resolved Outdated
glen commented 2025-02-26 00:24:31 +00:00
Outdated
Review
Copy link

Please refresh my memory: does this only fire when debugging is turned on in some way? If not, please file an issue to make debugging console logs compile in or out depending on something in the invocation.

Please refresh my memory: does this only fire when debugging is turned on in some way? If not, please file an issue to make debugging console logs compile in or out depending on something in the invocation.
Vectornaut commented 2025-02-27 21:07:52 +00:00
Outdated
Review
Copy link

Done, by filing issues #56 and #57.

Done, by filing issues #56 and #57.
self.regulators.with(|regs| {
for (_, reg) in regs.into_iter() {
@ -275,7 +310,9 @@ impl Assembly {
&JsValue::from(reg.subjects.0),
&JsValue::from(reg.subjects.1),
&JsValue::from(":"),
&JsValue::from(reg.set_point.get_untracked())
&JsValue::from(reg.set_point.with_untracked(
|set_pt| set_pt.spec()
))
);
}
});
@ -286,7 +323,7 @@ impl Assembly {
console::log_1(&JsValue::from(
format!("Updated constraint with subjects ({}, {})", subjects.0, subjects.1)
));
if set_point.with(|set_pt| set_pt.is_some()) {
if set_point.with(|set_pt| set_pt.is_present()) {
glen marked this conversation as resolved Outdated
glen commented 2025-02-26 00:32:47 +00:00
Outdated
Review
Copy link

I take it the .with method is how we manipulate the "payload" (or whatever the correct term is) that is wrapped by a Signal?
And I further take it that since the argument 0-ary function to create_effect only accesses the Signal set_point, the effect will only "fire" when that signal's "payload" changes?

I take it the `.with` method is how we manipulate the "payload" (or whatever the correct term is) that is wrapped by a Signal? And I further take it that since the argument 0-ary function to create_effect only accesses the Signal `set_point`, the effect will only "fire" when that signal's "payload" changes?
Vectornaut commented 2025-02-26 20:44:25 +00:00
Outdated
Review
Copy link

I take it the .with method is how we manipulate the "payload" (or whatever the correct term is) that is wrapped by a Signal?

Yes: with applies a function to the signal's value and returns the result. It also causes the signal to be tracked when it's called in a reactive scope. (Its documentation seems unhelpfully terse to me.)

And I further take it that since the argument 0-ary function to create_effect only accesses the Signal set_point, the effect will only "fire" when that signal's "payload" changes?

Yup! I agree that this is a bit subtle; I'm on the fence about whether to spell it out in a comment.

> I take it the .with method is how we manipulate the "payload" (or whatever the correct term is) that is wrapped by a Signal? Yes: [`with`](https://docs.rs/sycamore/latest/sycamore/reactive/struct.ReadSignal.html#method.with) applies a function to the signal's value and returns the result. It also causes the signal to be tracked when it's called in a reactive scope. (Its documentation seems unhelpfully terse to me.) > And I further take it that since the argument 0-ary function to create_effect only accesses the Signal set_point, the effect will only "fire" when that signal's "payload" changes? Yup! I agree that this is a bit subtle; I'm on the fence about whether to spell it out in a comment.
glen commented 2025-02-27 03:45:26 +00:00
Outdated
Review
Copy link

No need to comment it if that is the standard MO of the reactive package we are using.

No need to comment it if that is the standard MO of the reactive package we are using.
self.realize();
}
});
@ -308,15 +345,17 @@ impl Assembly {
let mut gram_to_be = PartialMatrix::new();
self.regulators.with_untracked(|regs| {
for (_, reg) in regs {
match reg.set_point.get_untracked() {
Some(set_pt) => {
let subjects = reg.subjects;
let row = elts[subjects.0].column_index.unwrap();
let col = elts[subjects.1].column_index.unwrap();
gram_to_be.push_sym(row, col, set_pt);
},
None => ()
}
reg.set_point.with_untracked(|set_pt| {
match set_pt {
Absent => (),
Present { value, .. } => {
let subjects = reg.subjects;
let row = elts[subjects.0].column_index.unwrap();
let col = elts[subjects.1].column_index.unwrap();
gram_to_be.push_sym(row, col, *value);
}
};
});
}
});

23
app-proto/src/outline.rs
View file

@ -10,9 +10,10 @@ use crate::{
AppState,
assembly,
assembly::{
ElementKey,
glen marked this conversation as resolved Outdated
glen commented 2025-03-08 00:26:29 +00:00
Outdated
Review
Copy link

Another use that needs to be harmonized with all our uses

Another `use` that needs to be harmonized with all our `uses`
Vectornaut commented 2025-03-08 23:31:30 +00:00
Outdated
Review
Copy link

Corrected, as discussed above.

Corrected, as discussed [above](#issuecomment-2317).
Regulator,
RegulatorKey,
ElementKey
SpecifiedValue::*
}
};
@ -20,14 +21,16 @@ use crate::{
#[component(inline_props)]
fn RegulatorInput(regulator: Regulator) -> View {
let valid = create_signal(true);
let value = create_signal(regulator.get_set_point_spec_clone_untracked());
let value = create_signal(
regulator.set_point.with_untracked(|set_pt| set_pt.spec())
);
// this closure resets the input value to the regulator's set point
// specification, which is always a valid specification
// specification
let reset_value = move || {
batch(|| {
valid.set(true);
value.set(regulator.get_set_point_spec_clone());
value.set(regulator.set_point.with(|set_pt| set_pt.spec()));
})
};
@ -40,10 +43,12 @@ fn RegulatorInput(regulator: Regulator) -> View {
r#type="text",
class=move || {
if valid.get() {
match regulator.set_point.get() {
Some(_) => "regulator-input constraint",
None => "regulator-input"
}
regulator.set_point.with(|set_pt| {
match set_pt {
Absent => "regulator-input",
Present { .. } => "regulator-input constraint"
}
})
} else {
"regulator-input invalid"
}
@ -51,7 +56,7 @@ fn RegulatorInput(regulator: Regulator) -> View {
placeholder=regulator.measurement.with(|result| result.to_string()),
bind:value=value,
on:change=move |_| valid.set(
regulator.try_specify_set_point(value.get_clone_untracked())
regulator.try_set(value.get_clone_untracked())
),
on:keydown={
glen marked this conversation as resolved Outdated
glen commented 2025-03-04 16:00:16 +00:00
Outdated
Review
Copy link

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.

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 2025-03-05 22:25:09 +00:00
Outdated
Review
Copy link

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) }

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:

valid.set(
    if let Ok(set_pt) = SpecifiedValue::try_from(value.get_clone_untracked()) {
        regulator.set_point.set(set_pt);
        true
    } else { false }
)

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.

> 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.
move |event: KeyboardEvent| {