Skip to content

feat: init binned spectrum and add axis conversions - #61

Draft
jamesrafe wants to merge 3 commits into
mainfrom
feat/init-binned-spectrum-and-add-axis-conversion
Draft

feat: init binned spectrum and add axis conversions#61
jamesrafe wants to merge 3 commits into
mainfrom
feat/init-binned-spectrum-and-add-axis-conversion

Conversation

@jamesrafe

Copy link
Copy Markdown

No description provided.

@fjebaker fjebaker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@icweaver
icweaver self-requested a review August 11, 2026 00:29
@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.75%. Comparing base (48a80f1) to head (0ab78c9).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #61      +/-   ##
==========================================
+ Coverage   94.50%   95.75%   +1.24%     
==========================================
  Files           9       11       +2     
  Lines         182      212      +30     
==========================================
+ Hits          172      203      +31     
+ Misses         10        9       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@icweaver

icweaver commented Aug 15, 2026

Copy link
Copy Markdown
Member

This is a really nice PR, thanks @jamesrafe! I think this would be a good opportunity to flex our ecosystem here and start factoring out some common functionality I've been seeing pop-up in a few of our packages.

For starters: @cgarling, what would you think about upstreaming your spectral density conversions in PhotometricFilters.jl to UnitfulEquivalences.jl? I've opened a PR to demonstrate what that might look like sostock/UnitfulEquivalences.jl#31

Next, I think we could replace the custom to_wavelength/to_frequency API introduced in this PR with simple methods extending Untiful.jl's uconvert. Something like this seems ergonomic to me:

uconvert(u"THz", spec) # Just convert the spectral axis
uconvert((u"keV", u"W/m^2/keV"), spec) # Convert the spectral AND flux axis

compared to the separate with_spectral_axis_and_flux_units()/with_spectral_axis_unit()/with_flux_unit() that astropy/specutils seems to juggle

There's also some cleanup in the binned spectrum logic I think we could apply here. This will all be included in my review, but just wanted to give y'all a heads up first on where I was thinking of taking this

@cgarling

Copy link
Copy Markdown
Member

No problem with moving the spectral density conversions if they accept the pr, IMO its a little more specialized than their other equivalences so IDK if they will want to add it

@icweaver

Copy link
Copy Markdown
Member

I may have been spoiled with the DynamicQuantities dev being so receptive to some of our astro-specific PRs in the past, so will see I guess 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants