Generalize constraints to observables #48
No reviewers
Labels
No labels
bug
design
duplicate
enhancement
maintenance
prospective
question
todo
ui
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: StudioInfinity/dyna3#48
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "Vectornaut/dyna3:observables_on_main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
The branch to be merged unifies the interface elements for measuring and constraining real-valued observables, as proposed in issue #47. Observables are presented as text inputs in the interface. When the observable is in measurement mode, the text field displays its value. Entering a desired value into the text field puts the observable in constraint mode. Setting the desired value to the empty string switches the observable back to measurement mode. If you enter a desired value that can't be parsed as a floating point number, the observable is set to constraint mode, but flagged as invalid.
Pitfalls
In this implementation of observables:
Review notes
The overall diff for this pull request includes a lot of line noise from renaming existing variables. The renaming is concentrated in commit
677ef47, so you should be able to focus on structural changes by looking at the diffs for the other commits individually.ObservableRolevariants af2724f934Another pitfall: If you have entered a collection of constraints that is unsatisfiable, then the current Assembly will have different values than the constraints for at least some of the measurements, but there will be no way to see what the actual values are. So it will be hard to gauge "how far off" the actual assembly is from the desired but unachievable one.
Do we want to consider addressing either of these pitfalls or any others that may come up in this PR? Or merely file them as issues when this PR is merged?
I think it makes more sense to file them as issues, because they seem like things that could be addressed by building on top of the framework for observables laid out in this pull request.
OK, as we discussed in meeting today:
(a) Fine to file these pitfalls as two WIP issues
(b) These controllable quantities created in this PR should be labeled in the display as some sort of a thing to kick off such labeling. We talked about 'cos θ' as one possibility. I also looked up what people call this quantity when it is bigger than 1 in magnitude. According to the Wikipedia page authors vary in whether it is always called the "inversive distance" (regardless of its magnitude) or whether when it is no larger than one in magnitude it is called 'cos θ' and when it is larger than one in magnitude it is called 'cosh δ' where δ is the thing that is called the "inversive distance" -- the rationale in the latter case being that δ is additive among spheres that have the same radical plane with (either of) these spheres as they have with each other, just like θ is additive among all spheres that go through the same intersection circle, in that regime. And so this latter convention is equivalent to calling it 'cos iδ', and then I suppose we could call 'iδ' the imaginary angle between the two spheres, even though I did not really find any reference that uses that terminology (I didn't search for a long time). That terminology actually makes a fair amount of sense, though, even though it doesn't seem to be much used, especially given that it corresponds very well to the situation of the circle of intersection becoming one point of intersection at tangency and then turning into an imaginary circle of intersection as the spheres move apart -- and this latter terminology is used, the "imaginary intersection" of two nonintersecting spheres, so why not "imaginary angle" between them by analogy?
So anyhow, please add a commit to this PR that calls it something, choosing your favorite among options like "cos θ" (where we can explain that θ is allowed to be pure imaginary = iδ), "inversive distance" (just calling it that always), or "cos θ or cosh δ" or something along those lines. It's likely just a temporary name anyway in the long run.
(I take it that with our normalization choices, the lorentz product of any two planes has magnitude at most 1? It's not that they coincide at product = 1 and then stay parallel but with separation at larger lorentz products, is it?)
(c) we should contemplate what we are calling these entities in the code. Some options:
I like the control theory flavor of "thermostat." Here are some more suggestions in that vein.
The term inversive distance sounds perfect for our use case. Nice find! With the current Lorentz product,
-Q(I, J)is the inversive distance between the spheres represented byIandJ. TheObservableInputcomponent showsQ(I, J), but maybe we can ignore the sign difference and call that the inversive distance for now.I've done this locally by hard-coding the label "Inversive distance" into the
ObservableOutlineItemcomponent, styled as shown in the attached screenshot. Is that good enough for this pull request? If so, I'll push it.I've added issues for both of the pitfalls we've identified and linked them from the pull request description.
I propose using the tag "prospective" for issues that would be introduced by pull requests under review. If you like that name, could you add the tag and assign it to each pitfall issue? As a placeholder, I've put "[prospective]" at the beginning of each issue title.
@Vectornaut wrote in glen/dyna3#48 (comment):
Done.
@Vectornaut wrote in glen/dyna3#48 (comment):
Sure, given that we will shortly add a radius Thermostat so that this PR is more or less just a mockup. But as a mockup, it occurs to me that once we switch to a split view of Entities in one pane and Dials in the other, then the constraints will have to be labeled something like
inversive distance(Deimos, Pollux)so if you want to switch to labeling along those lines so see how it looks now, that would also be fine with me.@Vectornaut wrote in glen/dyna3#48 (comment):
I agree that if we were showing the values graphically, e.g. a literal pointer to indicate an angle, or a slider for a radius, or a disk/rectangle scaled to indicate an area, then it would make perfect sense to overlay the current value and the set point so that when the constraint was on and satisfied, they would line up visually.
For numerical data, that visual layout does not work as well.
I am OK with a wide-ish text box where when a constraint is on and satisfied it just shows one number, but when it is suspended or unable to be satisfied it splits left/right and shows both the set point and the current value, with say the set point in red when it is supposed to be in force but currently violated, and maybe lighter/greyed out when it is suspended from having effect. And when something is just being measured, again only the measurement shows up. This scheme would suggest that when no constraining value has been specified, the current measurement should show up where the true measurement would be in the split view, and when one has been specified and is successfully being enforced, that value should show up where the set point does in the split view. I think that was one of your mock-ups, but not the one you pushed. If you like that as a future direction for how to deal with the pitfall issues, feel free to go back to that mockup, with/without any of the other contrast elements that are present in the current PR.
On the topic of what to name the class, I am OK with any of Dial, Thermostat, or Regulator, although I will say the last one speaks to me the least, perhaps because I have never used anything called a Regulator. You should also feel free to use a nonce word of your choosing, e.g., "Conquant" or "ConQuant", for constrainable quantity or "RWValue" for "read/write value" or whatever... I just think we should use a term that is nicely agnostic as to the direction of information flow.
Among those three, I'd be happy with "Regulator." I wouldn't be so happy with "Dial" (because it suggests a specific visual interface design) or "Thermostat" (because I'm used to it referring specifically to a temperature regulator).
How would you feel about "Constrainable"? We could contrast this with "Observable" for measurement-only variables, since I think you said that term has a read-only connotation to you.
I'd rather explore that labeling while designing the dual list component, since it might feel different in that context.
@Vectornaut wrote in glen/dyna3#48 (comment):
Sure. As I said, either way fine.
@Vectornaut wrote in glen/dyna3#48 (comment):
To me, "Constrainable" falls too much on the side of just constraining. Let's just go with "Regulator" then since we both have some level of contentment with that one. We can worry about what to name a measurement-only quantity once we have any, although off the top of my head, "Gauge" seems like the read-only analogue of a "Regulator" -- certainly you can have either a pressure gauge, which will only show you the pressure, and a pressure regulator, as you mentioned.
Regulatorfields b3e4e902f3The pull request should be ready for review again now! I've made the following changes:
I am still confused as to why Regulators have a role. It seems to me they always measure, so having the role not include measurement seems weird, and they constrain if and only if they have a setpoint. These considerations would seem to render the role superfluous?
Another approach I considered was changing the type of
set_pointtoSignal<Option<f64>>and using the following logic to determine the role.set_pointisSome, we're using the regulator as a constraint.set_pointisNone:set_point_textis the empty string, we're using the regulator as a measurement.set_point_textis non-empty, we're trying to use the regulator as a constraint, but the constraint is invalid because the set point couldn't be parsed.I decided that explicitly storing the role would make the code easier to understand, and that decoupling the role from the contents of
set_point_textwould make the code more flexible and less finicky. However, it wouldn't be too hard to switch to a non-explicit approach like the one described here.In my experience, having fewer places where the same information is kept or might be perceived to be kept actually makes the code easier to understand, as well as more concise (which also helps with clarity). There are fewer ways that things might get out of sync, or conversely, less work that needs to be done to keep the data in sync. That point of view supports removing the role if that information can clearly be deduced from the set point.
And along those lines,
f64s, which I am assuming are IEEE doubles, are already natively essentially Option values: NaN could very reasonably and clearly mean there is no current setting, without having to invoke the machinery (and verbosity) of Rust's Option generic.And more importantly, it now seems to me that set_point_text is very likely a violation of the Model-View-Controller separation of concerns. A Regulator is an entity in an Assembly, which is the platonic truth of some configuration we are studying with the software, and should only contain components of that platonic truth. So, what quantities are being regulated, and what those quantities are set to, are properly part of an Assembly. But a text representation of the set point of a Regulator belongs in whatever View might want to show that set point in that way, and then that view might have an entry box that could Control that set point, but if something invalid were put in the box, it simply would not change the set point; there would be no issue of the "text of the set point having been set to an invalid value". You can see this design philosophy in frontscope: when you enter an invalid value in a param entry box, there is simply no new value sent to the actual parameter in the visualizer/sequence code.
Or to put it another way: These inversive distance Regulators might well end up represented in a graphical view by little angle indicators (at least when the magnitude of the set point is ≤ 1). And quite plausibly, we might at some point want to enable dragging on the angle indicator to change the set point, just the way you can turn a thermostat. But that wouldn't mean we would put an "AngleDisplayWidget" here in the Regulator, correct? Similarly, it appears that having a text form of the set point in here is tied too tightly to the Table view (or whatever we are currently calling the text representation of the assembly), and it appears that it is an undesirable blurring of the separation of concerns between our Model (i.e., the Assembly) and our Views and Controllers (the table and/or graphical panel).
Please consider these aspects, minimizing redundancy (corresponding to maximizing DRYness) and clean separation of concerns, and refactor the Regulator-related code accordingly.
(I of course could be mistaken and there may be some good reason why a text representation of a floating point number set point is an inherent part of the platonic assembly of entities, so happy to discuss if you feel so.)
I know it might seem like I am making a big deal out of smallish points, but I think the beginnings of things are important and a good time to get the underlying structure of the code as "right" as can be foreseen at that time.
This field goes back to pull request #15, where it appears as the
lorentz_prod_textfield ofConstraint. If we change it, I'd recommend doing it in a separate PR, since it's compatible with the changes that this PR focuses on.According to my meeting notes from back then, we decided that the element and constraint structures should only include data that's shared across all views. As a user, I think it's helpful for a regulator's set point text to be shared across all views—both the text input views we have now and the angle display widgets we might have later. Similarly, if there's some extra data that should be shared across all angle display widgets, that extra information could also be stored in the constraint structure.
Some advantages of sharing set point text across all views
Handling invalid input transparently
Suppose I enter some invalid set point text in one view of a regulator. I'd like the invalid text to stay there so I can correct it, rather than disappearing. While the invalid text is there, is the constraint in force? If so, what set point is being used? I don't want the answers to depend on whether there are other views of the regulator, or on hidden information.
Sharing the set point text across all views accomplishes this. While the invalid text is there, the constraint is not in force, and you can see this in any view of the regulator by noticing the invalid text.
A possible alternate solution is to add a display of the actual set point to any view containing a failed attempt to enter a new set point. That would take up extra space in the interface, though.
Reducing ambiguity during text entry
If set point text isn't shared across views, typing into one view will temporarily put the interface in a state where different views show different set points. This shouldn't be a huge problem, because the views will all become consistent again when the
changeevent fires, but I do find it somewhat annoying and confusing.Okay, I'll switch to a system like the one described here.
I think that matching an
Optioncould make for more streamlined code in this case. I also prefer to treat NaN values as exceptions rather than using them to store information, partly because of all the caveats in thef64::NANdocumentation. If you strongly prefer it, though, the NaN approach should be just as easy to implement.We had a productive discussion on what data should be in a regulator. Aaron made the point that one could reasonably specify an inversive distance as either 0.24 or .24, and it would be weird if the system changed that for you. Moreover, an angle widget might reasonably set the inversive distance to some specification like 60°, or we might at some point allow specifications like 1/√2 or √2/2, and we would not want the system to alter our input either way from one to the other. Finally, one possible future way that we may want to implement linked constraints is by allowing Scalar elements, and if we have one named
t, perhaps you should be able to entertas the value -- and we certainly don't want that to just be converted into the current float value oft, as the point is we need all the constraints that havetin them to change in concert.These examples suggest that the set point of a regulator should be a text specification of its value, and that it should not have a floating-point set point at all; rather, the floating-point value of the set point will be computed (and perhaps cached internally if the consistency issues with that can be dealt with) when needed.
We'll try to finalize this discussion next time.
P.S. If the set point is a string, then there remains the same question as to whether the absence of a set point is the empty string, or if the set point is an
Option<string>. I am guessing in this case, the practicalities will come down on the side of using the empty string for no set point.Whoops—I've rediscovered why the
rolefield, or something equivalent, is needed to make regulator views act the way they currently do. This logic for determining a regulator's role would work ifset_point_text, likeset_point, were only updated when you commit the new set point by firing achangeevent. However, theset_point_textsignal is updated more frequently than that, because it's used to keep the set point's specification consistent across all views from keystroke to keystroke. Thus, seeing thatset_pointisNoneandset_point_textis non-empty doesn't always mean that the user has tried and failed to enter a new set point: it could also mean that the user is in the process of typing a set point for a regulator that doesn't currently have one.Storing role information elsewhere
We could still get rid of the
rolefield by upgradingset_pointfrom anOptionan enum with three variants:Behavior changes that would remove the need for role information
If we decide that the set point's specification should no longer be consistent across all views from keystroke to keystroke, maybe this logic would work. I still think that behavior change would be orthogonal to this purpose of this pull request, though.
Errata
I realized while writing this that during our meeting, and in the comments above, I kept saying "
inputevent" when I meant "changeevent." I've corrected the comments.Per our discussion, I feel like we are heading to a state where the set point is a string, and I thought you preferred the style of input in which each keystroke did not change the set point, but only the "commit" action - enter or change focus. Ergo, when you are typing in one control for the set point, the other displays of the set point (textual or graphical) should not change, as they are merely displaying the current set point; they are not involved in the uncompleted action of setting it. Only when you commit the change would all the other displays update. Similarly, as you are dragging a protractor widget, only that widget would show the potential new value -- only when you release the drag would the other displays update with the result of the gesture.
This is all hanging together for me, and I thought it was more or less what you were advocating. We can finalize the discussion Monday hopefully.
That seems reasonable. Should I try to lump that behavior change into this pull request? Or should I leave the behavior and
rolefield as-is for this pull request and open an issue (in the Demo project) for the input behavior and architecture overhaul?@Vectornaut wrote in glen/dyna3#48 (comment):
I'd love for this PR to get regulators to a solid prototypical behavior for future development, so yes, lumping it in would be great.
As for validation, I think it becomes clearer in this model. We can discuss further Monday, but I would propose that each regulator knows how to validate a possible set point, and a text-entry widget for a string set point would validate (only) per keystroke, indicating in some way when the current value in the box is a valid new set point. When it's not, committing is not possible: enter "klunks" (and if we use the "blur focus to indicate successful commit" analogy, the blur would not happen, or in general, whatever the indicator of a successful commit is, it would not happen), and we would need to decide if external focus change would discard the invalid input so far, or leave the invalid input in place with the invalidity indicator, or what, but the set point would not change and any other views of the same regulator would still show the set point that is still in force). This is similar to the situation in which one drags a protractor to an invalid spot (say to its hinge, where the angle is ambiguous) -- releasing there would have no effect on the current set point.
This validation scheme has the attraction that the actual set point in the Regulator can never be set to any invalid value. That clears the way for it to potentially in some future revision be some other, perhaps more abstract type that can only take on valid values, as long as it can faithfully reproduce the string that produced it. For example, if we do eventually have some expression language for valid set points, it could be a parse tree in that language, annotated with the characters that produced each node.
Just for reference/comparison on this aspect, both GeoGebra and Desmos allow you to set the coordinates of a point to a decimal string, and GeoGebra then canonicalizes that string (by eg. inserting a leading zero before the decimal place if there isn't one) but Desmos does not; it preserves your exact input. GeoGebra allows exponential notation like
3E-4for 0.0003, Desmos does not (it creates a slider namedEand sets that slider to an initial value of 1, so you get a coordinate intially equal to minus one but controllable by the E slider).Amusingly enough in GeoGebra when you enter
3E-4it canonicalizes it for display to0.0003, but if you then re-focus that text entry field for further typing, it is pre-filled with3*10^(-4)-- so it is clearly keeping both a "definition string" and an "actual value", but given that, it's weird that it doesn't preserve your exact input. So the canonicalization is occurring to the "definition string", and then the values are simply shown in the current preferred format (you can set that format to some extent, e.g. the number of digits; if you enter more digits than the current format, it keeps them in the definition string but rounds in the actual value display). And note that you can toggle the display of the elements between their actual values and their definition strings. But even in the latter case, canonicalization occurs from what you actually typed to the canonical definition string, as soon as you hit enter.So if we do at some point have a mode in which you can display both set points and actual values, it would be OK if a particular regulator showed .37 as its set point -- the analogue of GeoGebra's "definition string" for a Regulator -- if that's what the person typed, but 0.3700000001 (or 0.37) as its actual value; I think it is perfectly OK to have a canonical format to present result values, that does not need to depend on the definitions of entities.
I didn't check any other systems in this very brief survey of the field.
Not for this PR, but for the future on the point of auto-change-per-keystroke which I understand your reservations about -- if we have the ability, it could well be that a control widget in the midst of making a change to a Regulator, with a current uncommitted input that is nevertheless currently valid, could initiate a trial recompute and when that completes, if the current state of the control widget corresponds to the same value and is still uncommitted, is displayed as a preview in some way -- perhaps a ghost of the new configuration, either on its own or overlaid on the current configuration. That could be super useful. It answers a number of your reservations except for "it makes my computer thrash", but I think at some point we should still try such previewing, and throttle its resource consumption if necessary. GeoGebra, for example, does in many but not all circumstances, do such previewing. If we did it, we would want to be more consistent.
(In slightly more detail, it seems GeoGebra will preview when the current input constructs a new entity without changing any existing ones. So if I type
A = (1,1)and hit enter and then typeCircle(A, 3)even before I hit enter a preview circle of radius 3 about (1,1) appears, and if I backspace and change it toCircle(A, 2)the preview changes immediately. Then if I hit enter, the new circle is truly created and the display changes a bit accordingly. But now if I typeA=(2,3)there is no preview, even though if I hit enter the point A moves and the circle is re-drawn with its new center.)I like the way Desmos handles validation. When you enter an invalid expression, like
y=x&, it stays in the expression list with an invalidity indicator next to it, and it doesn't produce any output. Editing an expression from valid to invalid removes its output.Geogebra behaves similarly, although not as consistently. You can see this behavior by entering the expressions
A=(1,0),B=(0,1),Circl(A,B)in the algebra view (including the deliberate typo in the third expression).That's perfectly reasonable, and I think consistent with the above.
Regulator#36I've now implemented this behavior change in a way that I think is consistent with our meeting discussions on February 13 and 17. The implementation is on the branch
regulators-always-validin my version of the repository. Once you try it out and confirm that it's working the way you want, I'll fast-forward the pull request branch to match it.Summary of new behavior
Option<f64>.Some(x), the string parses to the floating-point valuex.None, the specification is empty.RegulatorInput, the regulator's set point gets updated, and allRegulatorInputviews of the same regulator update with it.RegulatorInput, the regulator's set point doesn't change. The input value is marked as invalid with an indicator icon and a distinctive color.RegulatorInputhas focus, pressing escape resets its value to the regulator's set point specification.Excellent, thanks. I'd first like to get straight the Regulator interface. Am I more or less correct in thinking of the pub items in the Regulator struct as the public member data, the other items in the Regulator struct as private member data, and the pub functions in the Regulator impl as the Regulator public methods?
Presuming so, I understand the subjects and measurement, and the validating
try_specify_set_pointmethod (with its slightly verbose name; I would have gravitated to something likeset, ortry_setto emphasize it could fail). But I am a bit unclear on the other data/methods. First, I think it is much clearer to think of the specification as the set point -- that's what you've "punched into the thermostat" -- and the resulting f64 as say theset_point_value, so I am going to use those terms and you can consider whether that renaming would make the Regulator structure and operation clearer. Clearly the engine needs to be able to get the set_point_value, and oh, I guess a graphical widget that wants to display the set point of the Regulator as a calipers or a slider bar needs the set_point_value. So OK, that's a public read-only value, makes sense.But anything that wants a textual display of the Regulator is interested in the set point itself, and is likely not to care at all about the set_point_value. So shouldn't the presence/absence of a set point be encoded in the set_point itself (the thing you are now calling set_point_spec) and not in the set_point_value (the thing you are now calling the set_point)? Why make a text-only control of the regulator need to deal with the set_point_value at all? I don't see generally speaking how it cares.
So if the source of knowledge of whether there is a set point becomes the set_point itself, then set_point_value could be simplified to a
ReadSignal<f64>. And then we would need to decide how the presence/absence of a set_point is encoded in the set_point entity, currently a String. My gut is whether the string is empty or non-empty, but maybe you have reasons to prefer an Option the way you preferred an Option over using NaN? Anyhow, let me know your thoughts on that.(As an aside, I am unclear on how the setting of the private data currently called set_point_writable is linked to clients getting the value from the public ReadSignal currently called set_point -- I'd be happy to be illuminated on this, although it's not explicitly germane to the design issue here. I am similarly unclear how the engine gets the measurement into a Regulator when that's a ReadSignal with no apparent affordance to write to it...)
And so that just leaves: how do clients get what I am calling the set_point (the string) and notifications when it updates? This certainly has to be read-only access; to update the set_point, clients must go through try_specify_set_point. I am assuming that's what the two methods
get_set_point_spec_cloneandget_set_point_spec_clone_untrackedaccomplish, but I am unclear on how this mechanism works and why two methods are needed. From my naive not-yet-familiar-with-Rust (or the reactive programming package we are working with) perspective I would have just expected something like aReadSignal<String>or maybe aReadSignal<Option<String>>if it's deemed better to use the Option to encode whether there is a set point at all. Thanks for some explanation of how this all works.Looking forward to understanding this all better; if you want to wait to discuss verbally on Thurs rather than write a long text response, that's fine.
Setting regulators' private fields
Yes, that's correct. Note that private items are visible throughout the module where they're defined (in contrast to the convention I'm most used to, where a structure's private items are only visible from its own methods). The function
insert_new_regulator, which is in theassemblymodule but isn't a method ofRegulator, takes advantage of this when it initializes a newRegulator, including the private fields. Outside theassemblymodule, it should be impossible to create aRegulatorthis way—a detail that will be relevant to your question about howset_point_writableis linked toset_point.A shorter name would be fine with me. Maybe we could use
try_specifyfor the method that takes a new set point specification, leavingsetavailable for a future method that would take a new set point value. This future method might be used, for example, by graphical views of the regulator.Set point specifications and values
I've been imagining that most internal operations will use the set point value, because I expect it to be more canonical and easier to extract information from. The set point specification, in contrast, might require complicated parsing to extract information (as it already does) and allow many ways of expressing the same set point value (although it doesn't yet).
To avoid confusion in references to the current source code, I'll keep using the field names
set_pointandset_point_specin this comment.Yes, the set point specification currently encodes whether or not there is a set point, as explained at the end of this section. If you'd like, I could rewrite the text-only
RegulatorInputview to avoid reading fromset_point: I'd just have to rewrite this closure to check whetherset_point_specis empty instead of checking whetherset_pointisNone.I chose the current version because I think it more clearly communicates that we're checking whether or not there's a set point. I don't think it costs us anything, because passing the whole regulator as an argument to
RegulatorInputmakes sense to me—and if we have it, we might as well take advantage of all its public data.Could you say more about what simplification you're proposing here? For me, two possibilities come to mind:
NaNvalues rather thanNoneto record the absence of a set point, as previously proposed.set_pointto have a value even whenset_point_specspecifies that there is no set point.However, neither of these feels like a simplification to me.
Yes, that's the scheme used in the current revision. It's described, somewhat implicitly, in my last comment and in the source code.
Getting regulators' private fields
As the Sycamore documentation explains: "A
ReadSignalcan be simply obtained by dereferencing aSignal. In fact, everySignalis aReadSignalwith additional write abilities!" Theset_pointfield always holds the underlyingReadSignalobtained by dereferencingset_point_writable. This invariant is enforced by the factory methodinsert_new_regulator, which is supposed to be the only way to create aRegulatorfrom outside of theassemblymodule.Sycamore's
ReadSignaltype provides several access methods. They come in pairs: a reactive version with a basic name and a non-reactive version with_untrackedat the end of the name. Right now, the only methods we use to access a regulator's set point specification areget_cloneandget_clone_untracked, so I found it convenient to write wrappers for these access methods:get_set_point_spec_cloneandget_set_point_spec_clone_untracked.Yes, exposing the set point specification's underlying
ReadSignalas a public field might be a cleaner solution. It would match what we currently do for the set point value.@Vectornaut wrote in glen/dyna3#48 (comment):
No.
I think we continue to be struggling with a possible non-DRY mental model of a Regulator. For the design to be maintainable in the long run, there can only be one authoritative location of the truth of what a Regulator is set to. You have convinced me, by the 0.24 vs .24 example (which I assume is already in effect), that such location cannot be an f64 -- it must be a parse tree or a string or a RegSpec or something. Therefore, any f64 accessible from a Regulator is only an efficiency/convenience read-only reflection of the underlying truth. (And yes, of course we do need a way to get that f64 that doesn't mean re-interpreting the authoritative set point every time.)
Therefore, there can never be any way to directly modify the f64 value of the set point of a Regulator. There could, at most, be a convenience method that converts an f64 in some canonical way into a candidate for being the authoritative set point of a Regulator.
So, a graphical control can take advantage of reading the f64 to decide what pixels to paint, but when a geometer person makes a gesture on that control that should be interpreted as modifying the set point, the graphical control must at that point create a new parse tree or string or RegSpec or whatever entity is an instance of the type of the authoritative set point, and attempt to set the Regulator to that. (This will likely in practice have some consequences along the lines of whenever you use a graphical control, the new set point is of the form 0.35, even if it had been .6 before.)
Is this making sense? Again, we can continue the discussion in person if it will be more productive. Thanks for bearing with it!
@Vectornaut wrote in glen/dyna3#48 (comment):
I disagree on both counts. It's the spec that is the set point, so looking at it is the clearest and cleanest way to check whether there is a set point. And if you can look at just one bit of a structure to do your work, that's always better than "taking advantage of all its public data". For one thing, it makes future refactors more convenient.
@Vectornaut wrote in glen/dyna3#48 (comment):
The simplification is precisely: only having one way to check whether there is a set point. If that way is checking whether the specification of the set point is empty, then that is the way. And if that's the way, then making the value of the set point be an
Option<f64>is non-useful redundancy at best and confusing at worst: could a regulator have a set point and yet not have a value? Or vice-versa? A future coder who is not you could rightly worry about such possibilities.So if no set point
\equiv"set point specification is empty," then an f64 for the value suffices, andOption<f64>is in any situation less simple than f64 when an f64 suffices. As for how to interpret said f64, the interpretation is: if there is no set point, the f64 is meaningless. if there is a set point, the f64 is the corresponding value for the Q-product of the subjects implied by the set point. Hence, there is no need to worry about what the f64 might or might not be when there is no set point; it's simply moot.If we had gone down a path like GeoGebra where it was OK for the system to canonicalize a widget input of .24 to an f64 printed representation like 0.24, then
Option<f64>might be a perfectly reasonable type for the actual set point -- it would need a bit for whether it has been set or not, which bit is supplied by the Option -- you made a plausible case thatisNaNmay in some ways be a less good source for that bit. And in that world, we would definitely not have a string in the Regulator, except possibly for a read-only "printed_form" string that was completely determined by the set point (and so could not have a 0.24/.24 alternation).But that's not the decision we took, and I am comfortable in a world in which the set point is some other specification. As I keep saying, it's reasonable for the set point to be what you "type into the thermostat" -- but then we need to keep clear and clean that said data item is the set point, and other information from the Regulator consists just of read-only consequences of that setting.
Yes, that makes sense. In the next revision of the code, I'll rename fields along the lines you suggested earlier to reflect this point of view. How about these names?
set_point_specset_pointset_pointset_point_parsedAdvantages of
_parsed:set_point_parsedis a transformation of the set point specification, cached for convenience.set_point_parsedmight become a tree, andset_point_evaluatedcould be a floating-point value that approximates the evaluation of that tree.If we go with
set_point_value, then at some point we should change the suffix_valthat I currently use to distinguish the contents of a Sycamore signal from the signal itself. We could switch to_eval, for example.I'm cool with those names -- I'll leave it up to you to decide whether it makes sense to just go to set_point_evaluated now and if we want to introduce something like a tree, use the name set_point_parsed later (the name does strongly suggest the value will be some sort of abstract syntax tree). Of course, it could also be that the set_point itself might in the future go from an unstructured string to an AST -- there's no reason its authoritative representation has to be a plain string, as long as the clients that need a string form can obtain the string from the set_point.
Aside: If we go that route, we may run into the canonicalization question again: if a geometer enters
1/ √2as the set point and hits enter, is it OK if that auto-changes to1 / √2, say? And note that even if we don't want this kind of canonicalization, the set_point still might not need to be just an unstructured string, as long as the exact input string is recoverable from the AST (maybe leaves are annotated with the exact characters that created them, and a concatenating traversal reconstructs the string; or maybe every node is annotated, and so the annotation of the root is the original string...).So anyhow, at the moment I am mellow about any names you prefer as long as they make it clear to the code reader which exact piece of information is the authoritative set point.
Single source of truth about whether there's a set point
I agree that it would be good to maintain a single approved way to check whether there's a set point. I propose making that a public method or read-only signal field of
Regulator. (The latter would let us write effects like this without cloning the wholeRegulator.)We can't use visibility to prevent future developers from inventing other ways to check, because we can't avoid making the set point specification publicly readable, but we can emphasize in the documentation that this method or field is the only way to check which is guaranteed to work.
Reflecting whether there's a set point in
set_pointhelps keep out bugsAn
Optionpushes you to check conditionsWhen
set_pointis anOption<f64>, there are only two ways to get anf64out of it:unwrap_or.unwraporexpect, which come with big red flags in the documentation. These functions tell you, as you're typing them, that you'd better make sure the data you're trying to extract is really there. At worst, if you try to use thef64when there's no set point, the program will panic rather than misbehaving more subtly.A bare
f64feels free to useWhen
set_pointis just anf64, using it without additional checks feels perfectly natural. It raises neither an error during compilation nor a red flag during code review. This makes it easier to use thef64in a situation where it's meaningless because there's no set point.Abstractly, the problem here is that conditional invariants are trickier to use correctly. That point of view is described next.
Reflecting whether there's a set point in
set_pointsimplifies invariantsThe current unconditional invariant
Right now, our code enforces a simple, unconditional invariant (documented here):
This invariant does lead to some redundancy, as you said: when
set_point_specsays there's no set point,set_pointhas to reflect that information. I think the simplicity of the invariant makes the redundancy worth it. Enforcing the invariant does come with a maintenance cost, but I'll argue that weakening the invariant also comes with a maintenance cost.The proposed conditional invariant
This would make the invariant above conditional:
We'd have to be careful to check the condition every time we use the invariant. This is feasible: we're already using conditional invariants elsewhere in the
assemblymodule. However, I do think it makes the code trickier to reason about, and it opens a door to bugs in the way described above.I will respond to each of your major headings in a separate post, just because of length.
@Vectornaut wrote in glen/dyna3#48 (comment):
It won't surprise you that I applaud this. It's the external/interface side of the single source of truth principle. But the internal side, that the truth only resides in one place, is important too for clarity/maintainability.
Right, there are two ways to go here: either the emptiness of some string is the record of whether there's a set point, in which case clients should use that (because I don't think there's any efficiency issue suggesting we need to cache the emptiness of that string), or the Regulator reserves the right to have the string be nonempty and yet there to be no set point, in which case it must provide some way to get at that truth. I'm fine either way on this.
EDIT: P.S. I am even fine with it currently being the case in actual operational fact that the string is empty iff there is no set point, even though we have recorded the existence of a set point elsewhere and/or provided a different way of checking. The latter design choice(s) just mean that we don't want the design of the Regulator to enshrine the emptiness as the official record of whether a set point is set -- perhaps because we think where that truth resides is likely to change in future code revision.
@Vectornaut wrote in glen/dyna3#48 (comment):
Perhaps. But it also helps create bugs and confusion -- it is an instance of the same information being stored in two places, and not for the sake of caching. It also forces client code to unwrap an Option, even when they know, because they checked in the sanctioned way, that there is a set point.
This caution is an excellent point, identifying a potential source of confusion in client code. It means it might be worth the extra cycles to set the f64 to NaN when there is in fact no set point -- not as a semantic guarantee, but as a safety measure. That setting would certainly prevent any use of a non-meaningful float value.
It's also obligatory: per your first major heading, there is only one valid/sanctioned way to tell whether there is a set point. We certainly don't want to force code that is complying with the documented interface by checking in the approved way to also have to go to the trouble of unwrapping an Option that it already knows must be a "Some" -- it's a distracting waste of code-reading time/effort and presumably of a couple cycles of the CPU.
This sort of redundant checking comes up all the time when writing typescript code: I have to do
instanceofchecks that I know from the logic of the code must be true in order to get the code to compile, because of course the TypeScript compiler is not a full-blown theorem prover about your code. It makes a lot of type inferences, but it doesn't, for example, check that the only events that are enabled on some element are ones where the target is an InputElement, say, so it barfs when you actually quite correctly invoke an InputElement method in the event handler. It's very frustrating. I definitely want to avoid this kind of redundancy in our code.That's a good point: the
assemblymodule's interface shouldn't push outside code to callunwraporexpect. Here's are two alternate proposals which address this problem.In these proposals, I'll use the set point specification to mean the source of truth about the set point, and set point data to mean data that depends only on the set point (obtained by parsing, evaluating, or otherwise processing the set point).
Proposal 1:
OptioneverywhereI like this idea. In fact, I think we should do it for every piece of set point data, including the set point specification. Wrapping all set point data in
Optionis a simple and consistent way to do this. It makes the safety measure stronger by panicking if we ever try to use a non-meaningful value. TheOption::mapmethod makes it easy—almost automatic—to guarantee that every piece of set point data is the sameOptionvariant as the set point. This leads to the following proposal.Option. We enforce the following invariant:Some(_)if and only if there's a set point.assemblymodule, we always do it usingOption::map, which acts on the contents ofSome(_)while preservingNone. As a result, set point data always comes wrapped in anOption, and we have the following invariant:Some(_)if and only if there's a set point.Option.Some(_)andNone. This code is an example of such processing.Proposal 2: specification only
If we do this, we lose a desired feature you mentioned earlier:
However, given the choice between
I'd lean toward spending the extra cycles.
OK, we'll discuss and attempt to choose between these proposals or some other consistent variation thereof today. Thanks for continuing to brainstorm here!
On Thursday (February 20), we decided to implement a variant of Proposal 1, which we called Proposal 1a. The idea was to make the set point an
Option<PresentSpecifiedValue>, wherePresentSpecifiedValuewould be a structure with private fields holding the specification and value of a real number.When I started to implement this proposal, I realized that it felt more natural to implement what I'll call Proposal 1b. I've made the set point a
SpecifiedValue, which is an enum with two variants:Absent, which indicates no value.Present, which holds a value and its specification.Proposal 1b shares the main safety advantage of Proposal 1a: code that uses the set point will only compile if it explicitly handles the absent case. It streamlines our code, relative to Proposal 1a, by reducing the number of "onion layers" around the set point, like you've been pushing for. It has three main disadvantages relative to Proposal 1a.
SpecifiedValuethat might bePresentis to call the associated functiontry_fromon a specification string. This ensures consistency between thespecandvaluefields. However, I don't think we can reasonably force client code to use the approved way: my understanding is that if we allow client code to match on thePresentvariant, we also have to allow client code to create thePresentvariant by hand.Presentvariant by hand. That could change, though.Option<PresentSpecifiedValue>matchesSome(pres_spec_val), we can pass downpres_spec_valto avoid redundant handling ofNone. In Proposal 1b, we can only pass down the individual fields of thePresentvariant.PresentSpecifiedValuestructure that converts easily to and from thePresentvariant.Option.Let's see if I understand. 1b replaces
Option<SetPointStruc>in which the Some() variant, as must be the case, has a single item payload which is a struct with spec and value, by basically a roll-our-own option in which the Present variant directly has two fields, spec and value. So in 1a, in the Some(setp) arm of a match, you must write setp.spec or setp.value to get at those things, whereas in 1b, in the Present{value,..} arm of a match, you have value directly and you can ignore the spec altogether if you like. Is that about it?If the 1b access feels more comfortable/streamlined, I am perfectly happy with it.
Not that I am advocating it, but could you get the enforced consistency back by making a setpoint a struct with one data member that was one of these present/absent gizmos and then making that data readonly from the outside, thereby forcing use of the setter method provided? That may of course be just as cumbersome as the option was, but I was just asking in principle could it be done that way.
Thanks for pursuing the optimal design here! I do hope it will pay dividends in dealing with the code as it evolves in the future.
Yes, that's one way Proposal 1b streamlines our code. However, there's another streamlining that I find more important: since the
SpecifiedValuetype is local to our crate, we can implement methods (specandis_present) and external traits (TryFrom) for it.Since
Optionis external to our crate, we can't directly implement methods or external traits forOption<PresentSpecifiedValue>. In my implementation of Proposal 1a, I use the newtype pattern, implementing the methods and traits mentioned above on a wrapper type:This has no performance cost, but in this case I think it adds a lot of noise to the code, because I'm writing stuff like this:
This would look nicer if we came up with a shorter name for the wrapper type. However, if the single-layer type in Proposal 1b works well enough already, I figure we might as well spare ourselves the trouble.
If I'm understanding this correctly, it sounds like another way of applying the newtype pattern. It should work, but it would add noise in the same way as described above.
OK, that's all meshing with my understanding. 1b is fine, I think we both prefer it at least slightly. See you later.
Regulatorbbd0835a8fI've fast-forwarded the pull request branch to Proposal 1b, so you can review and merge if you'd like.
This review is going to be big, just simply because this is sort of the first time that the app really "does something". So I am going to make several observations in this comment, and some of them should be addressed in this PR perhaps and others should become new issues once this is merged, or new issues right now if they don't really have to do with this PR. But I think they all need to be addressed in some way at some point.
aand hit enter. You get an error indicator in that one input box, and the other still looks fine, and that is all good. But there now doesn't seem to be anything you can do to just "cancel" the failed update: going into the other box and hitting enter has no effect; going into the error box and deleting everything and hitting enter unsets the regulator, and I couldn't think of anything else to try. This does not seem to be a case of needing our "undo" system, because in fact, no actual action has taken place (and frankly I would be fine if save, close, and reload did not restore the input boxes into this error state). There needs to be some means of simply restoring that control to its current actual state. We may need to discuss what that interaction should be.OK, here's a batch of code-level review along with the behavior-level comments I just made. This is all a lot of stuff, so once it's triaged and decided what will be addressed now and what will be moved to issues and what will be left as is, I will likely need to review again... thanks for understanding.
@ -27,3 +31,3 @@pub color: ElementColor,pub representation: Signal<DVector<f64>>,pub constraints: Signal<BTreeSet<ConstraintKey>>,pub regulators: Signal<BTreeSet<RegulatorKey>>,Is it worth commenting that this is intended to be the set of all regulators that have this Element as a subject? Or is that entirely clear? Similarly, I think it should be noted somewhere, perhaps as a comment here, the mechanism/responsibility for maintaining that information. When a regulator is inserted into an Assembly, is it the assembly's responsibility to tell each of the subjects? I imagine that's the design, but it would be good to have it written down somewhere.
I mean, of course by now I have gotten to that section of the code and seen that what I surmised is exactly what's happening, but I think the obligations of the various structures should be made clear not just in the implementing code in case some later coder comes along and doesn't realize that if the Assembly modifies regulators in some way, it's the Assembly's responsibility to update this information on the elements...
I've added a comment to this effect (
7cbd926). Let me know if I should make it more detailed.Detail level was fine. I just tweaked grammar/explicitness level. Resolving.
@ -115,1 +118,3 @@pub struct Constraint {// 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 ofTiny writing nit: this is a "garden path" phrasing -- I read "format discussed above" and started looking for the format specification earlier in the file, since I was pretty sure I hadn't seen one. Maybe reword just a bit to avoid that trap I fell into?
Nice catch! I've changed "above" to "at" to resolve the ambiguity (
7cbd926). Does that read all right to you?I agree unambiguous now. And hopefully this will change to an explicit anchor link when we get the doc system set up. So resolving.
@ -116,0 +137,4 @@//pub fn spec(&self) -> String {match self {Absent => String::new(),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.I definitely think we should give the
Absentvariant 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 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?
To me, it seems perfectly natural to have this:
An absent
SpecifiedValuehas only a specification. A presentSpecifiedValuehas both a specification and a value. The specification is always stored in the same place. When I write something likethe string
my_specis stored inspec_valand then returned byspec_val.spec(), regardless of whetherspec_valisAbsentorPresent.I thought the main point of the
SpecifiedValuetype 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 aSpecifiedValuedoesn't carry its specification along with it.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
specfield 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
NaNin 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.
If you find it less disorienting, we could do this:
Now both variants have a field called
spec, and thePresentvariant also has a field calledvalue. This would makematcharms that handleAbsentandPresentvalues look more similar.To access the specification of a general
SpecifiedValue, we'd still need to call thespec()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.
spec()method to access the specification field across all variants, is similar in spirit to this answer.Dereftrait forOptionalSpecifiedValue, kind of like in this answer. I switched to Proposal 1b before getting the trait working, though.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...
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 anOptionreturn type and returningNonewhen 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.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:
SpecifiedValuetype, instead of being loose in the regulator.NoneorSome(_).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.
I've switched to Proposal 1c (
84bfdef), which we adopted during today's meeting. TheSpecifiedValuestructure is read-only, courtesy of the readonly crate, so nothing you do with it outside thespecifiedmodule should be able to break the consistency between its fields. I've confirmed, for example, that outside thespecifiedmodule:SpecifiedValueby manually initializing its fields.SpecifiedValuecan't be assigned to or borrowed as mutable.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.
There's one more way: the
from_empty_specmethod is public too.Yup!
@ -116,0 +143,4 @@}fn is_present(&self) -> bool {match self {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.
Several enums from the standard library have similar methods.
Option:is_some/is_noneResult:is_ok/is_errCow:is_borrowed/is_ownedIn the tracking issue for those
Cowmethods, there's a hearty debate about whether such methods are a good idea. Pattern-matching has improved sinceOptionandResultwere 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.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
issyntax 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 anif letor amatches!()so that as Husht matures, the conversion will at some point rewrite this to theissyntax we both like better. Thoughts?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.
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
isoperator, and if there were, they would prefer that over the proliferation ofis_borrowedetc.@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.
Agreed. I think you get the tradeoff at this point, so let me know whether you want
SpecifiedValueto have anis_presentmethod in this pull request.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.
I've removed
if_present, replacing it withmatches!(/* ... */, Present { .. })where it was used (c58fed0).@ -116,0 +154,4 @@// 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 errorMaybe 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).
Done (
7cbd926).@ -121,0 +177,4 @@}impl Regulator {pub fn try_set(&self, set_pt_spec: String) -> bool {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...
We could indeed modify
try_setso that it returns a clone of theResult<SpecifiedValue>computed from the given specification. This might be useful if the caller wants a detailed error report when thetry_setfails. Right now, though, the code that callstry_setonly wants to know whether the attempt succeeded.The associated function
try_fromis a constructor, so it can't just return a success flag. Would revising the comment on theTryFromimplementation help communicate this? Readers already familiar with theTryFromtrait would know this already, but making documentation broadly accessible is often worthwhile.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?
Yes: the
set_pointfield is public. Thus, there's no need fortry_setto return something that gives you access to the set point. If you have access to a regulator'stry_setmethod, you also have access to itsset_pointfield.Here's the story as I vaguely understand it, based on my shaky knowledge of ownership in Rust. When the specification is valid,
try_fromreturns a result of the formOk(set_pt). We then callself.set_point.set(set_pt), which movesset_ptinto the signalself.set_point. That means we can't returnset_pt. I don't see a straightforward way to extract a reference to the value stored in a signal. I can useSignal::withto 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_setfails, we could havetry_setreturn aResult<(), ParseFloatError>.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.
Yes, I think we should add a
setmethod for this as soon as we have a reason to use it.Even if there were a
setmethod, though, I would still want to keeptry_setas a wrapper until we get to a point where we never have a reason to use it. I think thattry_setis a concise way to express an operation that feels natural and self-contained.I am not yet fluent in Rust syntax, so I am sure the below are not correct, but why does something like
need an abbreviation
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 overloadsetto 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 alreadysetting 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...Exactly.
As far as I know, Rust doesn't have function overloading, so the versions of
setthat take aSpecifiedValueand aResult<SpecifiedValue, ParseFloatError>would have to have different names. We could choosesetandset_if_ok, for example. I could look through the standard library to find out what kind of naming might be idiomatic.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.
Oh, this SO answer says the way to support
myReg.set(aPlainSpecifiedValue)andmyReg.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.The idea of using a parameterized trait is nice, but in this case I'd prefer to use methods named
setandset_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.Just here, or in the code too?
In the code, thanks.
Done (
894931a).@ -209,3 +275,1 @@let key = self.constraints.update(|csts| csts.insert(constraint));let subject_constraints = self.elements.with(|elts| (elts[subjects.0].constraints, elts[subjects.1].constraints)pub fn insert_regulator(&self, regulator: Regulator) {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 forinsert_new_regulator, and indeed, at the moment it is only called from there.Good point! I've made
insert_regulatorprivate for now (c368a38).@ -212,0 +275,4 @@pub fn insert_regulator(&self, regulator: Regulator) {let subjects = regulator.subjects;let key = self.regulators.update(|regs| regs.insert(regulator));let subject_regulators = self.elements.with(Isn't there a way in Rust to just do the equivalent of JavaScript's
forEachon 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?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
subjectsfield 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.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.
@ -215,0 +293,4 @@reps.0.dot(&(&*Q * reps.1))});let set_point = create_signal(Absent);Please illuminate me: Why do we not need to write
SpecifiedValue::Absenthere? I mean, I like the brevity, but I think I am unclear about when the bare variants are in scope...The declaration
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.Ah, the fact that
useis sort of "lost" in the midst of the file suggests that SpecifiedValue should be in its own source file and imported here, so the relevantuseis up where one would generally look. assembly.rs is getting pretty huge anyway... What are your thought about that factoring?Specified values and assemblies do seem like pretty independent concepts, so putting
SpecifiedValuein its own module might be reasonable.Are you doing so in this PR? That's my (relatively mild) recommendation.
Yes, I'll do this—I was just waiting for a go-ahead from you. I should've mentioned that in the last comment.
Done (
309b088).@ -215,0 +302,4 @@/* DEBUG */// print an updated list of regulatorsconsole::log_1(&JsValue::from("Regulators:"));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.
Done, by filing issues #56 and #57.
@ -215,0 +323,4 @@console::log_1(&JsValue::from(format!("Updated constraint with subjects ({}, {})", subjects.0, subjects.1)));if set_point.with(|set_pt| set_pt.is_present()) {I take it the
.withmethod 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?Yes:
withapplies 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.)Yup! I agree that this is a bit subtle; I'm on the fence about whether to spell it out in a comment.
No need to comment it if that is the standard MO of the reactive package we are using.
insert_regulatorprivate c368a38803assemblymodule 7cbd92618bYou can press escape to cancel the failed update, as described at the end of this comment. I've added a comment about this to issue #38, which mentions other hard-to-discover keyboard controls.
I've added your proposed fix as issue #59.
I've proposed a fix as issue #60.
I don't think these issues are relevant to this pull request. They arose long before the pull request branch, and I don't think they can be fixed in passing by changes this pull request would naturally make.
I've brought this up in issue #61.
Done (#62).
I've opened issue #63 to spotlight this, with references to relevant remarks in issue #44.
I think I've finished responding to this comment and addressing or responding to all the code-level comments. Please review again at your convenience! I'll be working on issue #64 in the meantime.
OK, I agree the "six-item" comment is handled, and I have gone through the outstanding conversations and resolved or replied to each one. Let's get them all to the point of resolution -- even if it is say the filing of another issue -- before I give the whole thing another full review. Thanks!
try_setwithset_if_ok894931a6e7SpecifiedValueinto its own module 309b0881dfis_presentutility method c58fed073dmatchtoif let8b4a72c60cSpecifiedValueas a read-only structure 84bfdefccbI've pushed changes that should resolve all the remaining conversations. If you agree, you can go ahead with another full review! Otherwise, let me know what's next for resolving the conversations.
@ -29,0 +58,4 @@bind:value=value,on:change=move |_| {let set_pt_result = SpecifiedValue::try_from(value.get_clone_untracked());valid.set(set_pt_result.is_ok());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
validto true and call the as-yet-unwritten regulator.set with that specified value, in that case, and in the alternative (Error) case, just setvalidto 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 whatif 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 simplersetmethod, or in fact maybe just a direct call ofregulator.set_point.set(...), eliminating the entireimpl Regulatorblock at lines 127-135 of assembly.rs, which seems like a win.So I guess I am specifically suggesting replacing this block with
-- presuming the syntax is OK -- and just removing that
impl Regulatorblock in assembly.rs altogether.That sounds great to me. I'd reorganize the block to avoid the repetition of
valid.set. Depending on whether you preferif letormatch, we could do either of these:In the
matchversion, the argument ofvalid.setis just an inlined version of the oldRegulator::try_setmethod. If we want to go this route, and you don't have a strong preference for inlining, I'd recommend just reverting commit894931a, which replacedRegulator::try_setwithRegulator::set_if_ok. We can also rewritetry_setto useif letinstead ofmatch, of course.Either of the last two is fine by me; I am not experienced enough with Rust to have formed a preference. So happy to defer to yours. All else being equal, the if let seems more concise. And as far as conciseness goes, what does the set_point.set() method return? If it happened to be "true" that would save two lines and a pair of braces in the match version, which would make that briefer. But this is a trivial point either way.
As far as whether to revive try_set: my philosophy is that if this is the only occurrence, then Occam says leave it inline; as soon as there are two (or more), DRYness says factor it into a regulator method. So unless you have a specific reason to (re-)create that regulator method, just leave it be.
p.s. it would be sort of nice, and reasonably intuitive, if the default else-branch of an if let was just
else {false}.try_setas inline code 6eeeb1c6fdDone (
6eeeb1c).If this is a Husht proposal, let's discuss it in the Husht repository.
@Vectornaut wrote in glen/dyna3#48 (comment):
Not necessarily, mostly just musing/wondering what Rust's behavior actually is when there is no else clause... if every expression/statement has a value, what is the value of an if let when there is no else and the match fails?
The value is
(), as described in the Rust reference:The compiler seems to treat every block as having the potential to be evaluated, forcing every non-unit
iforif letexpression to have anelseblock. For example, the expressionif true { 3 }won't compile unless you add anelseblock to convince the compiler that it has the same type in all situations.This is one advantage of using enums, by the way. To make the code below compile, you have to either change one of the matching blocks to an unconditional
elseor add an unreachableelseblock. I think both options make the expression harder to read and maintain.With an enum, you can write exactly what you mean.
@Vectornaut wrote in glen/dyna3#48 (comment):
Ah, right, because Rust doesn't have plain (non-discriminated) union types, and the if expression has to type. So if you wanted to return a u8 from one branch and a boolean from another branch, I guess you could define
(modulo some namespacing issues, likely) But I don't think you can pass a Flag() to something that wants a boolean, so this isn't too useful...
There are various ways to streamline the use of
MyBranchesvalues in conditionals. Which one I'd choose, if any, would depends on what the type is supposed to represent. Let's start with the base definition ofMyBranches:With this, we can already test
Flag(_)values using identifier patterns:If we want a
MyBranchesto be broadly interpretable as a boolean, we could implementDeref<Target = bool>for it:Then deref coercion would allow the compiler to see a
MyBranchesas aboolunder some circumstances. However, we'd still have to dereference explicitly to use aMyBranchesin anifexpression:I mean, a MyBranches value isn't like a reference to a bool, it's like a solid bool, with sometimes more information. So what we might want is to define an automatic type conversion (rather than a dereference). I don't think Rust goes in for that, although if I recall, C++ for example does. UPDATE: I have added a Husht item concerning this, and there's no need to discuss it further here.
@ -5,0 +5,4 @@engine,AppState,assembly::{Assembly,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.@ -12,0 +10,4 @@AppState,assembly,assembly::{ElementKey,Another
usethat needs to be harmonized with all ourusesCorrected, as discussed above.
@ -0,0 +18,4 @@impl SpecifiedValue {pub fn from_empty_spec() -> SpecifiedValue {SpecifiedValue { spec: String::new(), value: None }try_from("")? Wouldn't it be cleaner reuse either to implement this as the appropriate unwrapping of try_from(String::new()), or else in the try_from case implement the empty string case by returning the result of this function? We really only want the format of the canonical absent SpecifiedValue to be contained in one place, it seems to me.08ec838. If we eventually have more than one way to specify an absent value, it might make sense to switch to a "canonically specified absent value" function, but ensuring consistency withtry_fromwould get more complicated. For that reason, I'd want to make that switch in the context of actually wanting it, rather than guessing at what we might want in the future.from_empty_specdid have potential for inconsistency withtry_from. Commit08ec838implements your suggestion of havingtry_fromcallfrom_empty_specwhen the specification is empty.All right. Having the syntactic "trivial" generator of an unspecified SpecifiedValue (i.e. the one specified by an empty string) rather than a semantic one like "the canonical absent SpecifiedValue" seems a slightly odd emphasis to me (on syntax over semantics), but as you say, at the moment they are more or less equivalent because in fact there is only one syntactically correct specification of an absent SpecifiedValue. Hence resolving.
usedeclarations more compact b9db7a5699@ -27,2 +40,2 @@Err(_) => constraint.lorentz_prod_valid.set(false)};class=move || {if valid.get() {Please could you explain to me the three
moves in the three closures in thisview!macro, the one that specifies the class of the input element, the one that specifies its change handler, and the one that specifies its keydown handler?If I am understanding Rust correctly, the
moveannotation specifies that the closure should take ownership of any free variables that appear therein. So that would mean that the function that computes the class of the input takes ownership of the regulator. Why is that desired? Is it because the input element may not actually be realized until after the RegulatorInput function returns, and the ownership transfer extends the lifetime of the regulator until the closure has a chance to run and grab the info it needs to set the class (or perhaps actually, it has to persist that regulator indefinitely and then it can react to all future changes to the regulator)? The class-calculating closure of the input element seems like a slightly odd place for the long-term ownership of the regulator to reside, but maybe it doesn't really matter what owns it, as long as it persists indefinitely?But it seems my suggestions in the previous paragraph can't possibly be correct, because
regulatoralso occurs free in the closure passed ason:change, but the ownership of one entity can't exist in two places, if I am understanding correctly. So I guess please just explain to me what's going on here.And finally, the third
moveseems the most confusing. For the life of me, I can't see any free variables in the closure passed ason:keydown, so there doesn't seem to be anything to move, and so themoveshouldn't be there.Thanks in advance for illuminating these things for me.
Replying to your latest post here, to keep it in this resolvable conversation. Ah, the fact that Signal has Copy semantics makes this code much clearer. I didn't know that, and it seems a weird quirk of Rust to me that with ownership such a crucial concept to Rust, it is not clear whether copy or ownership transfer is in effect and I don't see any way you can know without consulting the source/docs for Signal.
Anyhow, I now get the first two
moves. And now I see thatreset_valueis just a local closure, and that's what's beingmoved by the thirdmove(for the keydown event). So that's good. Resolving.Don't think I ever saw a response to:
OK, I think those two things are the only unresolved conversations, as far as I can see. We'll get them wrapped up, I will try running the latest version again, and assuming I don't see anything odd, we'll get this merged shortly. Thanks!
Whoops—I've now responded in that discussion.
Yes, I think so. One way to see this is to compile with one of the
movekeywords removed. The compiler will then complain that the closure may outlive theRegulatorInputfunction, but it borrows some variables which are owned byRegulatorInput.I think this works because
Signalimplements theCopytrait, so the signalsvalue,valid, andregulator.set_pointare copied into the closures instead of being moved. Each closure ends up with its own copy of the signal, and each copy provides access to the same data. I think the following example shows the same phenomenon, because you get similar error messages if you remove any of themovekeywords.If you want to apply these comments to the situation in our code, keep in mind that the word "value" is a little overloaded. In these comments, it would refer to the signal that a variable is holding, not the value that the signal is holding.