-
Notifications
You must be signed in to change notification settings - Fork 1
Fix res2display() order cap and add km/m/cm unit ladder #102
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,10 +24,33 @@ def order2res(order): | |
| return res | ||
|
|
||
|
|
||
| def res2display(): | ||
| '''prints resolution levels''' | ||
| for res in range(20): | ||
| print(str(order2res(res)) + ' km at tessellation order ' + str(res)) | ||
| def res2display(max_order=MAX_ORDER): | ||
| '''prints resolution levels | ||
|
|
||
| Prints one line per tessellation order from 0 through ``max_order`` | ||
| (default MAX_ORDER = 29, the finest order the packed-u64 kernel reaches). | ||
| Each resolution is rendered in the largest sensible unit -- km at coarse | ||
| orders, m once it drops below 1 km, cm once it drops below 1 m -- and | ||
| rounded to three decimals within that bracket, so fine orders read | ||
| naturally (e.g. order 12 -> ``1.589 km``, order 13 -> ``794.456 m``) | ||
| rather than as tiny km fractions. | ||
|
|
||
| ``max_order`` must lie in 0..MAX_ORDER, the order range the packed-u64 | ||
| kernel encodes. | ||
| ''' | ||
| if not 0 <= max_order <= MAX_ORDER: | ||
| raise ValueError( | ||
| f"max_order must be between 0 and {MAX_ORDER}, got {max_order!r}") | ||
| for res in range(max_order + 1): | ||
| km = order2res(res) | ||
| if km >= 1.0: | ||
| value, unit = km, 'km' | ||
| elif km >= 1e-3: | ||
| value, unit = km * 1e3, 'm' | ||
| else: | ||
| value, unit = km * 1e5, 'cm' | ||
| print(str(round(value, 3)) + ' ' + unit + | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude (review) Nit / latent: bracket selection uses the raw Generated by Claude Code
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude Left standing intentionally. As the review notes, no order in the sanctioned Generated by Claude Code |
||
| ' at tessellation order ' + str(res)) | ||
|
|
||
|
|
||
| def unique2parent(unique): | ||
|
|
||
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.
🤖 from Claude (review)
max_orderis unvalidated at both ends. Two edge cases neither guarded nor tested:max_order→range(n<1)is empty, so the function silently prints nothing (e.g.res2display(-1)returns with no output, no error). Silent no-op on bad input is easy to misread as "nothing to display".max_order > 29→ happily prints beyond the kernel'sMAX_ORDER, contradicting the docstring's "29 = the finest order the kernel reaches". Worse, past order ~32 the cm bracket itself drops below 1 (0.152 cm,0.019 cm, …) — there's no mm rung, so it degrades to sub-1 cm fractions, the exact readability problem this PR set out to fix.Suggest clamping/validating to
0..MAX_ORDER(e.g. raiseValueErroron out-of-range, ormin(max_order, MAX_ORDER)), and add a test pinning the chosen behavior. Not a blocker for the sanctioned scope, but the docstring currently over-promises vs. the code.Generated by Claude Code