Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add sensitivity #377
Add sensitivity #377
Changes from 14 commits
bd59ae5
c117c1c
cc28086
48442e8
51bc009
58cd442
9da8266
7a1f1a1
eb24039
a1bb636
a11b5b2
e58d3d6
735ddc6
d014c66
8df5ab1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can replace
inverted
with a negative sensitivity value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do this mathematically, but I think it makes the API a lot less clear. I think we should keep them separate, and add some kind of non-negative constraint on sensitivity (or at least a warning).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, fair. Let's do that then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but what does "that" refer to? I'm guessing merging sensitivity and inverted as you said below, but what to do about making the API less clear, as plof27 said. I'm guessing just write useful docs on sensitivity to explain how inverting works now?I think this is addressed by leavinginvert()
functions for use, as it uses the same API as before. It's just inverted means reversing the sign instead of flipping a bool.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side note, instead of removing the invert functions, I can just invert the sensitivity to do the same thing.
This helps to keep the ergonomics of the invert bool without having it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I like that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, would you like that re-merged into one value? Or, close this as resolved and keep them separate. I am good with either solution.