Threshold condition #2305
Merged
Threshold condition #2305
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
jvesely
requested changes
Feb 6, 2022
There was a problem hiding this comment.
Other than the specific comments in the code, I have two high-level concerns.
1.) The attempt to handle int vs float values is incomplete. The only state params of int type should be execution counters, so we can just merge all special cases into one:
indices == TimeScale => value has is of type int => tolerance == 0.
2.) I'm don't quite like the custom handling of absolute and relative tolerance. The comparator logic would IMO be eaiser to explain if it used numpy's is_close.
== -- is_close
!= -- not is_close
>= -- '>' or is_close
> -- '>' and not is_close
<= -- '<' or is_close
< -- '<' and not is_close
This should also help with weird corner cases like:
val == threshold-epsilon # epsilon < tolerance
under the current semantics val > threshold returns true (i.e. threshold - epsilon > threshold - tolerance) which is confusing.
The above semantics with 'is_close' would return false.
You can construct a reverse example (val == threshold + epsilon) to point to a disrepance there, but I think it's easier to explain that values within tolerance are considered equal and the user should use '>=' in that case.
The main benefit of implementing semantics based on is_close is that it maintains traditional comparison invariants (e.g a > b => a != b) which is not true in the current version.
psyneulink/core/llvm/helpers.py
Outdated
| param_ptr, | ||
| [self.ctx.int32_ty(x) for x in ([0] + list(indices))] | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The above can be drastically shortened by reducing it to preprocessing indices,
The load statement would be more readable if it were reused and split into two expressions.
psyneulink/core/llvm/helpers.py
Outdated
| comparator = condition.comparator | ||
| indices = condition.indices | ||
|
|
||
| tolerance_value = condition.rtol * abs(threshold) + condition.atol |
There was a problem hiding this comment.
I think it'd be easier to use numpy is_close, rather than reimplementing own version of relative and absolute threshold. there are compiler helpers with the same behaviour as well (see helpers.is_close)
There was a problem hiding this comment.
It could replace some of the checks. I think a manual value would still need to be used for the strict inequalities, though I'm not really sure how much a tolerance even makes sense for those.
There was a problem hiding this comment.
agreed, I think the semantics need a bit of discussion.
using is_close in all of them made sense to me because it preserves the expected comparison invariants,
but at the same time breaking 'x + epsilon > x' is not very intuitive either.
I'm not sure what the user expectations are here, and how much we can restrict them.
Allowing only shape inequalities can be one way out, users can always combine scheduling decisions themselves.
psyneulink/core/llvm/helpers.py
Outdated
| return cmp_method(comparator, val, threshold) | ||
| else: | ||
| abs_method = self.ctx.get_builtin(abs_method_name, [val.type]) | ||
| val_thresh_diff = builder.call(abs_method, [builder.fsub(val, threshold)]) |
There was a problem hiding this comment.
this unravels the attempt to have general handling of ints vs floats. The abs method might be general, but you're using fsub which will fail for integers anyway.
There was a problem hiding this comment.
True, is it better to handle int/float separately instead of just converting int to float? If any/all of the comparisons are to be done with helpers.is_close, I think that just takes floats.
There was a problem hiding this comment.
I was thinking about it too. with the current default of fp64 it should be ok (all int32 values can be represented in fp64).
Switching to fp32 will introduce problems for values above 16.7M, which might be too low for lifetime execution count.
I think it'd make sense to special case execution counts as the only integer codepath and maybe restrict allowed tolerances (e.g. only integer absolute tolerance?)
similarly to above, I don't have a good idea about what the users would expect here.
Conversion to fp64 and using is_close should be safe, for all situations, but might be unnecessary.
There was a problem hiding this comment.
I'm fine with writing a special case for execution count if it's faster or otherwise better, but for now I've just converted
jvesely
reviewed
Feb 9, 2022
kmantel
force-pushed
the
threshold
branch
2 times, most recently
from
96b06cf
to
fd6af0f
Compare
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
1 similar comment
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
jvesely
approved these changes
Mar 1, 2022
35 tasks
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
|
This PR causes the following changes to the html docs (ubuntu-latest-3.7-x64): See CI logs for the full diff. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.