Generalize constraints to observables #48
|
|
@ -6,10 +6,8 @@ use crate::{
|
|||
AppState,
|
||||
assembly::{
|
||||
Assembly,
|
||||
|
glen marked this conversation as resolved
|
||||
Regulator,
|
||||
Element
|
||||
},
|
||||
engine::Q
|
||||
}
|
||||
};
|
||||
|
||||
/* DEBUG */
|
||||
|
|
@ -199,49 +197,8 @@ pub fn AddRemove() -> View {
|
|||
(subject_vec[0].clone(), subject_vec[1].clone())
|
||||
}
|
||||
);
|
||||
let measurement = state.assembly.elements.map(
|
||||
move |elts| {
|
||||
let reps = (
|
||||
elts[subjects.0].representation.get_clone(),
|
||||
elts[subjects.1].representation.get_clone()
|
||||
);
|
||||
reps.0.dot(&(&*Q * reps.1))
|
||||
}
|
||||
);
|
||||
let set_point = create_signal(None);
|
||||
state.assembly.insert_regulator(Regulator {
|
||||
subjects: subjects,
|
||||
measurement: measurement,
|
||||
set_point: set_point,
|
||||
set_point_spec: create_signal(String::new())
|
||||
});
|
||||
state.assembly.insert_new_regulator(subjects);
|
||||
state.selection.update(|sel| sel.clear());
|
||||
|
||||
/* DEBUG */
|
||||
// print updated regulator list
|
||||
console::log_1(&JsValue::from("Regulators:"));
|
||||
state.assembly.regulators.with(|regs| {
|
||||
for (_, reg) in regs.into_iter() {
|
||||
console::log_5(
|
||||
&JsValue::from(" "),
|
||||
&JsValue::from(reg.subjects.0),
|
||||
&JsValue::from(reg.subjects.1),
|
||||
&JsValue::from(":"),
|
||||
&JsValue::from(reg.set_point.get_untracked())
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
// update the realization when the regulator becomes
|
||||
// a constraint, or is edited while acting as a constraint
|
||||
create_effect(move || {
|
||||
console::log_1(&JsValue::from(
|
||||
format!("Updated constraint with subjects ({}, {})", subjects.0, subjects.1)
|
||||
));
|
||||
if set_point.with(|set_pt| set_pt.is_some()) {
|
||||
state.assembly.realize();
|
||||
}
|
||||
});
|
||||
}
|
||||
) { "🔗" }
|
||||
select(bind:value=assembly_name) { /* DEBUG */ // example assembly chooser
|
||||
|
|
|
|||
|
|
@ -5,7 +5,7 @@ use std::{collections::BTreeSet, sync::atomic::{AtomicU64, Ordering}};
|
|||
use sycamore::prelude::*;
|
||||
use web_sys::{console, wasm_bindgen::JsValue}; /* DEBUG */
|
||||
|
||||
use crate::engine::{realize_gram, local_unif_to_std, ConfigSubspace, PartialMatrix};
|
||||
use crate::engine::{Q, local_unif_to_std, realize_gram, ConfigSubspace, PartialMatrix};
|
||||
|
||||
// the types of the keys we use to access an assembly's elements and regulators
|
||||
pub type ElementKey = usize;
|
||||
|
|
@ -111,21 +111,38 @@ impl Element {
|
|||
}
|
||||
}
|
||||
|
||||
// `set_point_spec` must always be a valid specification of `set_point`
|
||||
// `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 │
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
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
Nice catch! I've changed "above" to "at" to resolve the ambiguity ( Nice catch! I've changed "above" to "at" to resolve the ambiguity (7cbd926). Does that read all right to you?
glen
commented
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.
|
||||
// └────────────┴─────────────────────────────────────────────────────┘
|
||||
#[derive(Clone, Copy)]
|
||||
pub struct Regulator {
|
||||
pub subjects: (ElementKey, ElementKey),
|
||||
pub measurement: ReadSignal<f64>,
|
||||
pub set_point: Signal<Option<f64>>,
|
||||
pub set_point_spec: Signal<String>
|
||||
pub set_point: ReadSignal<Option<f64>>,
|
||||
|
||||
set_point_writable: Signal<Option<f64>>,
|
||||
set_point_spec: Signal<String>
|
||||
}
|
||||
|
||||
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()
|
||||
}
|
||||
|
||||
|
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!
|
||||
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.set(set_pt.ok());
|
||||
self.set_point_writable.set(set_pt.ok());
|
||||
self.set_point_spec.set(spec);
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
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
Several enums from the standard library have similar methods. In the tracking issue for those 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
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 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
I think it's best to keep Husht as close as possible to Rust in terms of features, for two reasons.
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
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 > 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
@Vectornaut wrote in glen/dyna3#48 (comment):
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
Agreed. I think you get the tradeoff at this point, so let me know whether you want > 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
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
I've removed I've removed `if_present`, replacing it with `matches!(/* ... */, Present { .. })` where it was used (c58fed0).
|
||||
true
|
||||
}
|
||||
|
|
@ -227,6 +244,54 @@ impl Assembly {
|
|||
subject_regulators.1.update(|regs| regs.insert(key));
|
||||
}
|
||||
|
||||
pub fn insert_new_regulator(self, subjects: (ElementKey, ElementKey)) {
|
||||
// create and insert a new regulator
|
||||
let measurement = self.elements.map(
|
||||
move |elts| {
|
||||
let reps = (
|
||||
elts[subjects.0].representation.get_clone(),
|
||||
elts[subjects.1].representation.get_clone()
|
||||
);
|
||||
reps.0.dot(&(&*Q * reps.1))
|
||||
}
|
||||
);
|
||||
let set_point_writable = create_signal(None);
|
||||
let set_point = set_point_writable.split().0;
|
||||
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())
|
||||
});
|
||||
|
||||
/* DEBUG */
|
||||
// print updated regulator list
|
||||
console::log_1(&JsValue::from("Regulators:"));
|
||||
self.regulators.with(|regs| {
|
||||
for (_, reg) in regs.into_iter() {
|
||||
console::log_5(
|
||||
&JsValue::from(" "),
|
||||
&JsValue::from(reg.subjects.0),
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
Should this really be Should this really be `pub`? Where would a client of Assembly get a "free-floating" regulator to insert? Seems, at least for the moment, like a private helper for `insert_new_regulator`, and indeed, at the moment it is only called from there.
Vectornaut
commented
Good point! I've made Good point! I've made `insert_regulator` private for now (c368a38).
|
||||
&JsValue::from(reg.subjects.1),
|
||||
&JsValue::from(":"),
|
||||
&JsValue::from(reg.set_point.get_untracked())
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
Isn't there a way in Rust to just do the equivalent of JavaScript's Isn't there a way in Rust to just do the equivalent of JavaScript's `forEach` on the two entries of subjects, looking the entry up in elts and updating the regulators of each by inserting key in them? I mean, rather than non-DRY-ly having to write two identical update calls?
Vectornaut
commented
I think tuples are designed with arbitrary combinations of types in mind, so I'd be surprised to find utility methods to broadcast over tuples of the same type—and indeed, I haven't managed to find any. If we change the I think tuples are designed with arbitrary combinations of types in mind, so I'd be surprised to find utility methods to broadcast over tuples of the same type—and indeed, I haven't managed to find any.
If we change the `subjects` field from a tuple to a fixed-length array, as I'm expecting to do when we address issue #55, we can make this code less repetitive by iterating over the array.
glen
commented
Ah, this strongly suggests that the subjects field should be a fixed-length array, if there is no way to iterate over the entries of a tuple. So let's make that a definite part of the plan for #55, and I'll resolve this here. Ah, this strongly suggests that the subjects field should be a fixed-length array, if there is no way to iterate over the entries of a tuple. So let's make that a definite part of the plan for #55, and I'll resolve this here.
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
// update the realization when the regulator becomes a constraint, or is
|
||||
// edited while acting as a constraint
|
||||
create_effect(move || {
|
||||
console::log_1(&JsValue::from(
|
||||
format!("Updated constraint with subjects ({}, {})", subjects.0, subjects.1)
|
||||
));
|
||||
if set_point.with(|set_pt| set_pt.is_some()) {
|
||||
self.realize();
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
// --- realization ---
|
||||
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
Please illuminate me: Why do we not need to write 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
The declaration puts all variants in scope. I currently have it just after the definition of 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
Ah, the fact that 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
Specified values and assemblies do seem like pretty independent concepts, so putting Specified values and assemblies do seem like pretty independent concepts, so putting `SpecifiedValue` in its own module might be reasonable.
glen
commented
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
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.
Vectornaut
commented
Done ( Done (309b088).
|
||||
pub fn realize(&self) {
|
||||
|
|
|
|||
|
|
@ -20,14 +20,14 @@ use crate::{
|
|||
#[component(inline_props)]
|
||||
fn RegulatorInput(regulator: Regulator) -> View {
|
||||
let valid = create_signal(true);
|
||||
let value = create_signal(regulator.set_point_spec.get_clone_untracked());
|
||||
let value = create_signal(regulator.get_set_point_spec_clone_untracked());
|
||||
|
||||
// this closure resets the input value to the regulator's set point
|
||||
// specification, which is always a valid specification
|
||||
let reset_value = move || {
|
||||
batch(|| {
|
||||
valid.set(true);
|
||||
value.set(regulator.set_point_spec.get_clone());
|
||||
value.set(regulator.get_set_point_spec_clone());
|
||||
})
|
||||
};
|
||||
|
||||
|
|
|
|||
In assembly.rs, you put all of the sub-namespaces under a top-level item on a single line, like
whereas here this has been spread across three lines. They should be consistent in format. If it's the same to you, I prefer a formats that balances the desire for fewer linebreaks (to keep a good amount of information visible on screen at one time) with the need for clear organization and readability (which too few linebreaks can engender). That is, I prefer the assembly.rs format.
Good catch. My convention has been to start with each
usedeclaration on one line. If that line gets too long, I switch the outermost braced list from space-separated to newline-separated. Then I do the same thing recursively at lower list levels. Theusedeclarations you noticed break this convention; I've corrected them In commitb9db7a5. I think the convention is followed everywhere else, but I'll keep an eye on it whenever I reviseusedeclarations in future commits.