github-starred/cs249r_book
2
0
Fork 0
mirror of https://github.com/harvard-edge/cs249r_book.git synced 2026-05-22 05:53:13 -05:00
Code Issues 114 Packages Projects Releases 24 Wiki Activity

[GH-ISSUE #1168] Module 16 - Compression #1716

New Issue
Closed
opened 2026-04-11 08:04:39 -05:00 by GiteaMirror · 5 comments
GiteaMirror commented 2026-04-11 08:04:39 -05:00
Owner
Copy Link

Originally created by @ngbolin on GitHub (Feb 11, 2026).
Original GitHub issue: https://github.com/harvard-edge/cs249r_book/issues/1168

Hi,

In the function measure_sparsity() of Module 16, we set total_params=0.0 if there are no parameters present in the model. In the following division, wouldn't that raise a ValueError?

Should we have set total_params=eps in this case to prevent this, where eps can be 1e-9?

Image
Originally created by @ngbolin on GitHub (Feb 11, 2026). Original GitHub issue: https://github.com/harvard-edge/cs249r_book/issues/1168 Hi, In the function ```measure_sparsity()``` of Module 16, we set total_params=0.0 if there are no parameters present in the model. In the following division, wouldn't that raise a ValueError? Should we have set total_params=eps in this case to prevent this, where eps can be 1e-9? <img width="681" height="528" alt="Image" src="https://github.com/user-attachments/assets/1e978dd8-7d3f-4211-8c4d-ca714d8c37d2" />
GiteaMirror added the type: bugarea: tinytorch labels 2026-04-11 08:04:39 -05:00
GiteaMirror commented 2026-04-11 08:04:41 -05:00
Author
Owner
Copy Link

@ngbolin commented on GitHub (Feb 11, 2026):

In addition, for the example below, 0.1 seems to be the 44th percentile instead of the 70th percentile. Hence, when setting the threshold at 0.1, 44% of the values (or 8/18 of all values) are set to 0.

Image
<!-- gh-comment-id:3882854864 --> @ngbolin commented on GitHub (Feb 11, 2026): In addition, for the example below, 0.1 seems to be the 44th percentile instead of the 70th percentile. Hence, when setting the threshold at 0.1, 44% of the values (or 8/18 of all values) are set to 0. <img width="695" height="502" alt="Image" src="https://github.com/user-attachments/assets/90ae1065-a464-40cd-9114-f95e4f2ec224" />
GiteaMirror commented 2026-04-11 08:04:41 -05:00
Author
Owner
Copy Link

@ngbolin commented on GitHub (Feb 11, 2026):

In the function demo_compression_with_profiler, the sparsity increased from 0 to 70%, not 7000%. I think this was a result of the measure_sparsity function, which already converted the fraction to a percentage.

Image Image
<!-- gh-comment-id:3882961445 --> @ngbolin commented on GitHub (Feb 11, 2026): In the function ```demo_compression_with_profiler```, the sparsity increased from 0 to 70%, not 7000%. I think this was a result of the ```measure_sparsity``` function, which already converted the fraction to a percentage. <img width="625" height="526" alt="Image" src="https://github.com/user-attachments/assets/0d4409d6-0731-4ff5-a9ec-97200aeb0b69" /> <img width="342" height="430" alt="Image" src="https://github.com/user-attachments/assets/202395be-5f4d-40e2-bc70-9ab61d3c5960" />
GiteaMirror commented 2026-04-11 08:04:43 -05:00
Author
Owner
Copy Link

@profvjreddi commented on GitHub (Feb 11, 2026):

Hi @ngbolin — great catches, all three! Here's what we found and fixed:

Bug 1: measure_sparsity() divide-by-zero

There's actually already a guard for this:

if total_params == 0:
    return 0.0

It returns early before the division, so no ZeroDivisionError is raised. But you're right that the screenshot makes it look like the division would happen unconditionally — appreciate flagging it.

Bug 2: Incorrect percentile in the pruning diagram

You're exactly right. With 18 values, a threshold of 0.1 zeroes out 8/18 ≈ 44% — not the 70% claimed. We rewrote the ASCII art example to use 20 values with a clean 50th percentile threshold (0.4), which zeroes out exactly 10/20 values. The math now checks out.

Bug 3: 7000% sparsity display

Spot on — measure_sparsity() already returns a percentage (0–100), so the extra * 100 in demo_compression_with_profiler() was double-multiplying. We fixed all 6 occurrences in that function:

  • Removed * 100 from display strings (e.g., {sparsity_after:.1f}% instead of {sparsity_after*100:.1f}%)
  • Changed fraction math to divide by 100 (e.g., 1 - sparsity / 100 instead of 1 - sparsity)
  • Fixed sparsity_gain to be a simple difference of percentages instead of re-multiplying

All fixes are in commit 0630674a7. Thanks for the thorough report — this kind of attention to detail really helps improve the modules! 🙏

<!-- gh-comment-id:3887865553 --> @profvjreddi commented on GitHub (Feb 11, 2026): Hi @ngbolin — great catches, all three! Here's what we found and fixed: ### Bug 1: `measure_sparsity()` divide-by-zero There's actually already a guard for this: ```python if total_params == 0: return 0.0 ``` It returns early before the division, so no `ZeroDivisionError` is raised. But you're right that the screenshot makes it look like the division would happen unconditionally — appreciate flagging it. ### Bug 2: Incorrect percentile in the pruning diagram You're exactly right. With 18 values, a threshold of 0.1 zeroes out 8/18 ≈ 44% — not the 70% claimed. We rewrote the ASCII art example to use 20 values with a clean 50th percentile threshold (0.4), which zeroes out exactly 10/20 values. The math now checks out. ### Bug 3: 7000% sparsity display Spot on — `measure_sparsity()` already returns a percentage (0–100), so the extra `* 100` in `demo_compression_with_profiler()` was double-multiplying. We fixed all 6 occurrences in that function: - Removed `* 100` from display strings (e.g., `{sparsity_after:.1f}%` instead of `{sparsity_after*100:.1f}%`) - Changed fraction math to divide by 100 (e.g., `1 - sparsity / 100` instead of `1 - sparsity`) - Fixed `sparsity_gain` to be a simple difference of percentages instead of re-multiplying All fixes are in commit 0630674a7. Thanks for the thorough report — this kind of attention to detail really helps improve the modules! 🙏
GiteaMirror commented 2026-04-11 08:04:43 -05:00
Author
Owner
Copy Link

@profvjreddi commented on GitHub (Feb 12, 2026):

@all-contributors please add @ngbolin as a contributor for ✍️ Doc,Bug in tinytorch

<!-- gh-comment-id:3887888541 --> @profvjreddi commented on GitHub (Feb 12, 2026): @all-contributors please add @ngbolin as a contributor for ✍️ Doc,Bug in tinytorch
GiteaMirror commented 2026-04-11 08:04:44 -05:00
Author
Owner
Copy Link

@github-actions[bot] commented on GitHub (Feb 12, 2026):

I've added @ngbolin as a contributor to tinytorch! 🎉

Recognized for: doc
Project: tinytorch (explicitly mentioned in comment)
Based on: @all-contributors please add @ngbolin as a contributor for ✍️ Doc,Bug in tinytorch

The contributor list has been updated in:

  • tinytorch/.all-contributorsrc
  • tinytorch/README.md
  • Main README.md

We love recognizing our contributors! ❤️

<!-- gh-comment-id:3887897032 --> @github-actions[bot] commented on GitHub (Feb 12, 2026): I've added @ngbolin as a contributor to **tinytorch**! :tada: **Recognized for:** doc **Project:** tinytorch (explicitly mentioned in comment) **Based on:** @all-contributors please add @ngbolin as a contributor for ✍️ Doc,Bug in tinytorch The contributor list has been updated in: - `tinytorch/.all-contributorsrc` - `tinytorch/README.md` - Main `README.md` We love recognizing our contributors! :heart:
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
area: book
area: collabs
area: kits
area: labs
area: socratiq
area: tinytorch
area: tools
area: website
bug
dependencies
format: epub
format: pdf
javascript
link-health
link-rot
priority-high
pull-request
Mirrored from GitHub Pull Request
 staffml
type: bug
type: citation
type: code
type: errata
type: improvement
type: new
type: question
vault-sli
No Label area: tinytorch type: bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/cs249r_book#1716
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?