LockableKnob init fix #193
Conversation
There was a problem hiding this comment.
Overall looks good, nice documentation and examples. The one area I wanted to discuss was to explore improving the API interface for selecting from a list of choices by configuring the knob with the length of choices instead of requiring the user to perform the percentage calculations.
| .with_locked_knob("scale", initial_percentage_value=initial_scale_percent) | ||
| .with_locked_knob( | ||
| "length", | ||
| initial_percentage_value=(self.LENGTH_CHOICES.index(initial_length) * 2 + 1) |
There was a problem hiding this comment.
[discussion] I think it would make more sense to take len(CHOICES) and do percent calculations internally. From a user perspective, I think using a percent as an input for selecting a choice from a list is less clear than configuring the lockable knob to choose from value from a range of len(CHOICES).
Why not accept either len(CHOICES) as the configuration input and do the percentage calculations internally?
Perhaps I don't fully understand the problem you encountered that inspired this change, but I do still see value in simplifying the API for usability sake with CHOICE knobs.
There was a problem hiding this comment.
So I did start going down this road earlier in the development of LockableKnob but I backed off of it because of the flexibility that the Knob api allows. Specifically, the api allows the client to read the knob for any number of choices at any time. So given that, saving the value for 'choice 2' felt weird to me.
That said, thinking about your suggestion more and looking at this code here, I think that you are right and we could make a 'choices' api for lockable knob that would make sense and be way cleaner than this mess.
I think that this change would be an improvement beyond the scope of this PR however, would you be ok with it coming in a followup PR?
There was a problem hiding this comment.
Yeah, I've found knobs to mostly represent "int", "percent", or "choice". If LockableKnob had an interface for each of those, I think that would help with the API clarity and usability.
| self.kb2 = ( | ||
| KnobBank.builder(k2) | ||
| .with_disabled_knob() | ||
| .with_locked_knob("p4", initial_percentage_value=0.5, threshold_from_choice_count=7) |
There was a problem hiding this comment.
[optional] nit: If you define choice_p4 as a instance variable and use len(choice_p4) here, that will remove magic numbers and make the example more clear.
There was a problem hiding this comment.
The intention is that this file isn't merged into the repo. I'll remove it after approval.
| self, | ||
| knob: Knob, | ||
| initial_percentage_value=None, | ||
| initial_uint16_value=None, |
There was a problem hiding this comment.
[optional] nit: This is Python, just int is fine, since there is no other int type. If the concern is clearly communicating a max value of uint16, that could be in the documentation. That might make the api interface a little more pythonic.
There was a problem hiding this comment.
The naming of this comes directly from the MAX_UINT16 constant in europi.py. And yes the intention is to describe this parameter as much more constrained than an int. I agree that the name is verbose, however the problem that this PR is trying to resolve is the confusion over exactly what this value represents. Putting it in the parameter name is my way of solving that.
This makes the values used in initial knob setup more clear.
add ability to set the initial value in the TM class. Use this to more clearly setup the inital state of the locked knob.
|
I am workin on resolving these conflicts. |
We'd like to make this available, but there seems to be a memory allocation issue when adding more scripts. This will be addressed in a different PR.
mjaskula
force-pushed
the
mjj/knob_init_fix
branch
from
9f526c5
to
675aaff
Compare
The range for setting the initial value of a LockableKnob was unclear and hard to use. This PR attempts to alleviate both of these issues. In addition it updates the Turing machine and knob playground to support these changes.
Details:
The internal representation of a LockableKnob's value uses the full AnalogueReader range of 0-MAX_UINT16. This was unclear in the LockableKnob's documentation and api. To fix this we've updated the API to make it clear how the value is being used by giving two possible ways. First is the original full int range, second is a percentage. The int range will be useful when saving and loading a knob value. The percentage will be more useful when hard coding a knobs initial state.
I've again included the 'KnobPlayground' script to aid in PR review and testing. It should be removed before merge.