Conversation
DanielaBreitman
commented
Apr 9, 2026
DanielaBreitman
commented
- fixes issues due to changes in powerbox API [[BUG] Incompatible with latest powerbox #81]
- fixes a few other issues (see comments)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #82 +/- ##
==========================================
- Coverage 85.27% 84.84% -0.44%
==========================================
Files 12 12
Lines 781 772 -9
Branches 192 190 -2
==========================================
- Hits 666 655 -11
- Misses 50 51 +1
- Partials 65 66 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Now also fixes [#77], but no test for it yet. |
steven-murray
left a comment
steven-murray
left a comment
There was a problem hiding this comment.
Thanks @DanielaBreitman ! Just a few niggly comments.
| k_weights_1d: Callable | None = ignore_zero_ki, | ||
| bin_ave: bool | None = True, | ||
| interp: bool | None = None, | ||
| interp: str | None = None, |
There was a problem hiding this comment.
| interp: str | None = None, | |
| interp: Literal['linear', 'nan-aware'] | None = None, |
| mu_min: float | None = None, | ||
| bin_ave: bool = True, | ||
| interp: bool | None = None, | ||
| interp: str | None = None, |
There was a problem hiding this comment.
| interp: str | None = None, | |
| interp: Literal['linear', 'nan-aware'] | None = None, |
| mu_min: float | None = None, | ||
| bin_ave: bool | None = True, | ||
| interp: bool | None = None, | ||
| interp: str | None = None, |
There was a problem hiding this comment.
| interp: str | None = None, | |
| interp: Literal['linear', 'nan-aware'] | None = None, |
| bins_kpar: int | un.Quantity | None = None, | ||
| log_kpar: bool | None = False, | ||
| interp_kpar: bool | None = False, | ||
| interp_kpar: str | None = None, |
There was a problem hiding this comment.
Can this also be nan-aware? Docs don't mention so?
There was a problem hiding this comment.
Looks like the answer is no. In this case this should just be Literal['linear'] | None
| ) | ||
|
|
||
| final_nmodes = np.sqrt(kperp_grid**2 + kpar_grid**2) | ||
| final_nmodes = kperp_nmodes_grid * kpar_nmodes_grid |
There was a problem hiding this comment.
Are we sure that this is the correct calculation? It smells wrong to me. At the very least, it should have a substantial comment above it, explaining the reasoning.
| ps, | ||
| kperp, | ||
| kpar, | ||
| nbins=16, | ||
| weights=1, | ||
| interp=False, | ||
| interp=None, | ||
| mu_min=None, | ||
| generator=None, | ||
| bin_ave=True, |
There was a problem hiding this comment.
Would be great if these were typed properly. Actually at this stage it might be useful to define the interp type (Literal['linear', 'nan-aware'] | None) separately as a new type.
2 participants
