Remove MapData #4720
Open
Remove MapData #4720
+572
−766
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
…p" (facebook#4719) Summary: Pull Request resolved: facebook#4719 D81363344 / Ax facebook#4255 disallowed having map keys other than "step", providing deserialization support, with warnings, when a map key other than "step" is deserialized. This was released in Ax 1.1.2; we are now on Ax 1.2.1. Since there have been warnings for a while about this, it should be save to remove the deserialization support. With this change, if a progression was saved with some other column name, such as "epoch," it will be deserialized as another MapData column, with NaN imputed for "step." When this logic goes away in D89820078, it will be deserialized as Data with no "step" column. Differential Revision: D89888874 Privacy Context Container: L1307644
meta-cla
bot
added
the
CLA Signed
|
@esantorella has exported this pull request. If you are a Meta employee, you can view the originating Diff in D89814417. |
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Dec 29, 2025
Summary: Pull Request resolved: facebook#4720 Changes: * Remove `MapData` * Replace MapData with Data everywhere * `isinstance(data, MapData)` becomes `data.has_step_column` everywhere * Documentation and messages to the user are updated accordingly * Update some documentation * Decoded MapData JSON to Data Follow-up diffs: * Remove the remaining functions in map_data.py, and MAP_KEY * Move MapData tests to Data Differential Revision: D89814417
esantorella
force-pushed
the
export-D89814417
branch
from
d040159 to
17f7efa
Compare
Summary: Pull Request resolved: facebook#4715 This diff merges MapData into Data by giving Data an attribute `has_step_column`. MapData becomes an empty subclass which will be removed in a subsequent PR (D89814417). Note on how this diff is split up: In this diff, functions that previously required MapData now can consume Data, and type checks stop referencing MapData. However, functions can still return MapData; all references are removed in the next diff. Changes: * Functionality from map_data.py is moved into data.py, and functionality from MapData moves into Data; MapData is an empty subclass. * Data (and MapData) get an attribute `has_step_column`; the important distinction becomes `has_step_column`, not type. * It is now possible for both `Data` and `MapData` to either have a step column or not. Having both `Data` and `MapData` like this is an unpleasant intermediate state that we should move off of immediately (landing this and D89814417 together) * Many `isinstance` checks become `has_step_coumn` checks * Data's new methods `subsample` and `latest` get special cases for when there is no "step" column * Make Data's required columns always be the same and not contain "step" (since it isn't really required). Remove `required_columns` method. Differential Revision: D89820078 Privacy Context Container: L1307644 Reviewed By: lena-kashtelyan
Summary: Pull Request resolved: facebook#4720 Changes: * Remove `MapData` * Replace MapData with Data everywhere * `isinstance(data, MapData)` becomes `data.has_step_column` everywhere * Documentation and messages to the user are updated accordingly * Update some documentation * Decoded MapData JSON to Data Follow-up diffs: * Remove the remaining functions in map_data.py, and MAP_KEY * Move MapData tests to Data Question: * Should we leave MapData in place, but have it error and tell people to use Data when initialized? Or is MapData sufficiently rarely used that a message to Ax developers is enough? My take: it's easy enough to do both Differential Revision: D89814417
esantorella
force-pushed
the
export-D89814417
branch
from
17f7efa to
232a19b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4720 +/- ##
==========================================
- Coverage 96.72% 96.72% -0.01%
==========================================
Files 582 582
Lines 60711 60643 -68
==========================================
- Hits 58722 58655 -67
+ Misses 1989 1988 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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:
Changes:
MapDataisinstance(data, MapData)becomesdata.has_step_columneverywhereFollow-up diffs:
Differential Revision: D89814417