Refactor _auto_bar_width and add _auto_bar_width_columnar#348
Closed
talgalili wants to merge 2 commits intofacebookresearch:mainfrom
Closed
Refactor _auto_bar_width and add _auto_bar_width_columnar#348talgalili wants to merge 2 commits intofacebookresearch:mainfrom
talgalili wants to merge 2 commits intofacebookresearch:mainfrom
Conversation
Summary: Refactored the `_auto_bar_width` function to simplify its interface by removing the unused `n_datasets` parameter, and extracted the columnar layout logic into a new dedicated `_auto_bar_width_columnar` function. The original `_auto_bar_width` was trying to serve two different purposes but the `n_datasets` parameter wasn't actually used in the calculation. This change: - Simplifies `_auto_bar_width` for single-bar-per-line layouts (grouped barplots and histograms) - Adds `_auto_bar_width_columnar` specifically for side-by-side columnar layouts used by `ascii_comparative_hist` - Improves code clarity by having separate functions with clear docstrings for each use case - Removed Overriding n_bins and bar_width example from balance_ascii_plots.ipynb Reviewed By: talgalili Differential Revision: D94465988
|
@talgalili has exported this pull request. If you are a Meta employee, you can view the originating Diff in D94465988. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the ASCII plotting functions to better separate concerns and simplify the bar width calculation logic. The main goal is to distinguish between single-bar-per-line layouts (grouped barplots and histograms) and columnar side-by-side layouts (comparative histograms).
Changes:
- Simplified
_auto_bar_widthby removing the unusedn_datasetsparameter - Added dedicated
_auto_bar_width_columnarfunction for columnar layouts with proper space accounting for multiple columns - Changed
ascii_plot_distto useascii_comparative_histfor numeric variables instead ofascii_plot_hist, resulting in a different output format (columnar vs. multi-line)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| balance/stats_and_plots/ascii_plots.py | Refactored _auto_bar_width to remove unused parameter, added _auto_bar_width_columnar for columnar layouts, updated ascii_plot_dist to use ascii_comparative_hist, updated docstrings and examples |
| tests/test_ascii_plots.py | Updated tests to call _auto_bar_width with one parameter, added tests for _auto_bar_width_columnar, updated expected outputs to match new comparative histogram format |
| tutorials/balance_ascii_plots.ipynb | Removed section 6 about overriding n_bins and bar_width parameters, consolidated tutorial introduction |
Comments suppressed due to low confidence (1)
balance/stats_and_plots/ascii_plots.py:786
- This is a breaking change in the output format for numeric variables in ascii_plot_dist. Previously it used ascii_plot_hist which shows each dataset on a separate line within each bin. Now it uses ascii_comparative_hist which shows all datasets as columns on the same line. According to the coding guidelines (review checklist item 5), breaking changes must be explicitly labeled in the CHANGELOG with migration notes and before/after examples. This change should be documented in the CHANGELOG.md file under "Breaking Changes".
ascii_comparative_hist(
meta-codesync bot
pushed a commit
that referenced
this pull request
Feb 26, 2026
Summary: Refactored the `_auto_bar_width` function to simplify its interface by removing the unused `n_datasets` parameter, and extracted the columnar layout logic into a new dedicated `_auto_bar_width_columnar` function. The original `_auto_bar_width` was trying to serve two different purposes but the `n_datasets` parameter wasn't actually used in the calculation. This change: - Simplifies `_auto_bar_width` for single-bar-per-line layouts (grouped barplots and histograms) - Adds `_auto_bar_width_columnar` specifically for side-by-side columnar layouts used by `ascii_comparative_hist` - Improves code clarity by having separate functions with clear docstrings for each use case - Removed Overriding n_bins and bar_width example from balance_ascii_plots.ipynb Differential Revision: D94465988 Pulled By: sahil350 fbshipit-source-id: fad39e4a7ebbf90dbc9e3c9d2df28672147c7145
Collaborator
|
Closing this PR since the changes have already been committed. |
Contributor
Author
|
Thanks @neuralsorcerer |
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
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.
Summary:
Refactored the
_auto_bar_widthfunction to simplify its interface by removing the unusedn_datasetsparameter, and extracted the columnar layout logic into a new dedicated_auto_bar_width_columnarfunction.The original
_auto_bar_widthwas trying to serve two different purposes but then_datasetsparameter wasn't actually used in the calculation. This change:_auto_bar_widthfor single-bar-per-line layouts (grouped barplots and histograms)_auto_bar_width_columnarspecifically for side-by-side columnar layouts used byascii_comparative_histReviewed By: talgalili
Differential Revision: D94465988