Remove extraneous functions from map_data.py #4721
Closed
+677
−768
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
|
@esantorella has exported this pull request. If you are a Meta employee, you can view the originating Diff in D89829345. |
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Dec 29, 2025
Summary: Pull Request resolved: facebook#4721 These became trivial when Data merged with MapData Differential Revision: D89829345 Privacy Context Container: L1307644
esantorella
force-pushed
the
export-D89829345
branch
from
2e0cc48 to
174f83d
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: saitcakmak, lena-kashtelyan
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Jan 2, 2026
Summary: Pull Request resolved: facebook#4721 These became trivial when Data merged with MapData Reviewed By: saitcakmak Differential Revision: D89829345 Privacy Context Container: L1307644
esantorella
force-pushed
the
export-D89829345
branch
from
174f83d to
474d6ab
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4721 +/- ##
=======================================
Coverage 96.72% 96.72%
=======================================
Files 582 582
Lines 60744 60720 -24
=======================================
- Hits 58753 58733 -20
+ Misses 1991 1987 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
esantorella
force-pushed
the
export-D89829345
branch
from
474d6ab to
818630b
Compare
esantorella
added a commit
to esantorella/Ax
that referenced
this pull request
Jan 2, 2026
Summary: Pull Request resolved: facebook#4721 These became trivial when Data merged with MapData Reviewed By: saitcakmak Differential Revision: D89829345 Privacy Context Container: L1307644
Summary: Pull Request resolved: facebook#4720 Changes: * Replace `MapData` with an empty class that just errors and says to use Data instead * Replace MapData with Data everywhere * `isinstance(data, MapData)` becomes `data.has_step_column` everywhere * Documentation and messages to the user are updated accordingly * Updated all documentation, including code comments * 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 Privacy Context Container: L1307644 Reviewed By: mgarrard, saitcakmak
Summary: Pull Request resolved: facebook#4721 These became trivial when Data merged with MapData Reviewed By: saitcakmak Differential Revision: D89829345 Privacy Context Container: L1307644
esantorella
force-pushed
the
export-D89829345
branch
from
818630b to
f5b831e
Compare
|
This pull request has been merged in 854455e. |
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: These became trivial when Data merged with MapData
Differential Revision:
D89829345
Privacy Context Container: L1307644