Use floating-point map key types in Benchmark MapMetrics #4126
Conversation
This file contains hidden or 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
meta-cla
bot
added
the
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D79938692 |
ltiao
added a commit
to ltiao/Ax
that referenced
this pull request
Aug 11, 2025
Summary: ### Context In the benchmarking set-up, the `normalize_progression=True` option in early-stopping strategies is currently not working properly as zero trials are early-stopped. Specifically, it appears that `is_eligible_any` returns `False` for all trials. All early-stopping strategies (all subclasses of `BaseEarlyStoppingStrategy`) are impacted. Specifically the instantiation of `MapData` in the return call of `_check_validity_and_get_data` casts the progression column to `int64`, which rounds all the float-valued progressions `[0.0, 1.0)` to `0`, leading `is_eligible_any` to always return `False` ### Changes This diff removes the specification of `map_key_info` as integer-typed in `BenchmarkMapMetric` and `BenchmarkMapUnavailableWhileRunningMetric`. Differential Revision: D79938692
ltiao
force-pushed
the
export-D79938692
branch
from
a7d3214 to
cfe771a
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D79938692 |
ltiao
added a commit
to ltiao/Ax
that referenced
this pull request
Aug 11, 2025
Summary: Pull Request resolved: facebook#4126 ### Context In the benchmarking set-up, the `normalize_progression=True` option in early-stopping strategies is currently not working properly as zero trials are early-stopped. Specifically, it appears that `is_eligible_any` returns `False` for all trials. All early-stopping strategies (all subclasses of `BaseEarlyStoppingStrategy`) are impacted. Specifically the instantiation of `MapData` in the return call of `_check_validity_and_get_data` casts the progression column to `int64`, which rounds all the float-valued progressions `[0.0, 1.0)` to `0`, leading `is_eligible_any` to always return `False` ### Changes This diff removes the specification of `map_key_info` as integer-typed in `BenchmarkMapMetric` and `BenchmarkMapUnavailableWhileRunningMetric`. Differential Revision: D79938692
ltiao
force-pushed
the
export-D79938692
branch
from
cfe771a to
82c74c1
Compare
Summary: ### Context In the benchmarking set-up, the `normalize_progression=True` option in early-stopping strategies is currently not working properly as zero trials are early-stopped. Specifically, it appears that `is_eligible_any` returns `False` for all trials. All early-stopping strategies (all subclasses of `BaseEarlyStoppingStrategy`) are impacted. Specifically the instantiation of `MapData` in the return call of `_check_validity_and_get_data` casts the progression column to `int64`, which rounds all the float-valued progressions `[0.0, 1.0)` to `0`, leading `is_eligible_any` to always return `False` ### Changes This diff removes the specification of `map_key_info` as integer-typed in `BenchmarkMapMetric` and `BenchmarkMapUnavailableWhileRunningMetric`. Differential Revision: D79938692
ltiao
force-pushed
the
export-D79938692
branch
from
82c74c1 to
a788189
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D79938692 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
This pull request has been merged in 67f0d60. |
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Aug 18, 2025
Summary: **Context**: Map values must satisfy two properties: * They need to be ordered such that it makes sense to take the largest value and modeled that or to sort on those values. * They shouldn't be typed as ints, since as we learned in D79938692 / Ax facebook#4126, this can result in floats getting cast to ints later. * They shouldn't be weird since they need to be serializable. This leaves one obvious candidate: float. Fortunately, existing usages seem to be floats. **This PR**: * Makes all map keys floats * Removes some type annotation logic that is now extraneous * Renames a silly test class (I had used "TestMapDataXYZ" to make it easier to run several related groups of tests via CLI then forgot to change it back.) Differential Revision: D80473359
facebook-github-bot
pushed a commit
that referenced
this pull request
Aug 18, 2025
Summary: Pull Request resolved: #4164 **Context**: Map values must satisfy two properties: * They need to be ordered such that it makes sense to take the largest value and modeled that or to sort on those values. * They shouldn't be typed as ints, since as we learned in D79938692 / Ax #4126, this can result in floats getting cast to ints later. * They shouldn't be weird since they need to be serializable. This leaves one obvious candidate: float. Fortunately, existing usages seem to be floats. **This PR**: * Makes all map keys floats * Removes some type annotation logic that is now extraneous * Renames a silly test class (I had used "TestMapDataXYZ" to make it easier to run several related groups of tests via CLI then forgot to change it back.) Reviewed By: saitcakmak Differential Revision: D80473359 fbshipit-source-id: 333bf0a97dfbfafa9e9921476b366b148812a3ed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary:
Context
In the benchmarking set-up, the
normalize_progression=Trueoption in early-stopping strategies is currently not working properly as zero trials are early-stopped. Specifically, it appears thatis_eligible_anyreturnsFalsefor all trials.All early-stopping strategies (all subclasses of
BaseEarlyStoppingStrategy) are impacted. Specifically the instantiation ofMapDatain the return call of_check_validity_and_get_datacasts the progression column toint64, which rounds all the float-valued progressions[0.0, 1.0)to0, leadingis_eligible_anyto always returnFalseChanges
This diff removes the specification of
map_key_infoas integer-typed inBenchmarkMapMetricandBenchmarkMapUnavailableWhileRunningMetric.Differential Revision: D79938692