-
Notifications
You must be signed in to change notification settings - Fork 6
Add mem_size histogram #619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #619 +/- ##
==========================================
+ Coverage 92.57% 92.64% +0.06%
==========================================
Files 32 32
Lines 1926 1998 +72
==========================================
+ Hits 1783 1851 +68
- Misses 143 147 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
delucchi-cmu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two small comments. Otherwise LGTM!
Co-authored-by: Melissa DeLucchi <[email protected]>
The second of three planned PRs for LSDB's #449.
Overall plan:
A little more detail:
threshold_mode. The threshold mode defaults torow_count, but is set tomem_sizeifbyte_pixel_thresholdis in the input args (and is notNone).MEM_SIZE_HISTOGRAM_BINARY_FILEandMEM_SIZE_HISTOGRAMS_DIR.gather_plan, which creates histogram directory(/ies) among other set up stuff.map_to_pixels); and in doing so, we create the histogram. We passthreshold_modetomap_to_pixels, and if it'smem_size, we additionally make themem_sizehistogram._get_mem_size_of_chunkand its two helpers_get_row_mem_size_data_frameand_get_row_mem_size_pa_tablewhich_histogramtoread_histogramto show that we're reading therow_counthistogram. It's the default, but I wanted to include it for readability/safety.