Generalize constraints to observables #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 #48 (comment):
Done.
@Vectornaut wrote in #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 #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 #48 (comment):
Sure. As I said, either way fine.
@Vectornaut wrote in #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 #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.
Constraint#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 #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 #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 #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 #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 #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:
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: 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!
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.