Passband/PassbandGroup new interface

[OliviaLynn]

Passband and PassbandGroup now follow a new interface:

source = Source(...)
passband = Passband(delta_wave: float | None = 5, trim_percentile: float | None = 0.1)
spectral_flux = source.evaluate(passband.waves, t)
band_flux = passband.flux_to_bandflux(spectral_flux)

Summary

After some iteration, we decided we could avoid the need for any flux interpolation/extrapolation if we:

1. Initialize our Passband (or PassbandGroup) object early on
2. Get the wavelengths used in its transmission table(s)
3. Feed those wavelengths to our model's evaluate method
4. Generate a spectral flux/flux density matrix that is already on the same grid (and shares the same interval) as our target passband

Note that in step 1, we can also specify a target grid step we would like to be using (additionally, we can fine-tune trimming the tails of our transmission tables via the trim_quantile parameter).

Both of these parameters may be reset at any time via process_transmission_tables, which will re-interpolate the transmission table(s) as needed.

Note:

• I have updated the snia unit test to conform to this new interface, and it runs successfully when redshift=NumpyRandomFunc("uniform", low=0.01, high=0.02), is replaced with redshift=0.
  I do not believe the failure here to be related to passbands.
• See second-to-last commit (or, second-to-last-before-PR-comment-commits) 805e8a7 for unit tests passing with this change made (however, I have since returned redshift back to NumpyRandomFunc).
• Considering the best course of action to be addressing this as part of the scope of Move redshift to Physical Model #76, but open to suggestion

Issue

Will close #64

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[github-actions]

Before [fba3f99]	After [2dca05f]	Ratio	Benchmark (Parameter)
1.19±0.01s	1.19±0.02s	1	benchmarks.time_x1_from_hostmass
7.43±0.07ms	7.22±0.05ms	0.97	benchmarks.time_chained_evaluate

Click here to view all benchmarks.

[hombit]

Great, thank you so much! Everything looks great, the only large thing is actually my fault and asks to rename trim_percentile everywhere.

[jeremykubica]

This looks like it isn't unique per function call. If so, it would be more efficient to do these operations once when setting up the data structure. That would mean keeping a separate dictionary of full_name to (lower_index, upper_index), so you'd need to look at how much time it saves for the added complexity.

[OliviaLynn]

Referenced in #112

[jeremykubica]

Again, I think this could be precomputed during data creation and stored in a dictionary mapping full_name to a boolean indicating that the arrays are equal.

[OliviaLynn]

Referenced in #112

[jeremykubica]

Should we just use astropy units here so we can use all their conversions?

[mi-dai]

I think we've decided to not do conversions at this stage. The problem is the LSST filters online are in "nm", so we would have to do the conversion offline instead of downloading it directly

[OliviaLynn]

Referenced in #113

[jeremykubica]

I think it is fine to keep this as-is, but an alternative approach that might be more modular approach might be to have the constructor take a table and then to add two class methods:

@classmethod
def from_file(cls, table_path: str, ...):
   load the file
   ...
   return Passband(table, ...)

@classmethod
def from_url(cls, table_url: str, force_download: bool, ...):
    do the download
    ...
    return Passband(table, ...)

[OliviaLynn]

Referenced in #113

[hombit]

Thank you!

[mi-dai]

I think we've decided to not do conversions at this stage. The problem is the LSST filters online are in "nm", so we would have to do the conversion offline instead of downloading it directly

[mi-dai]

I'm a little confused why the check would fail in these scenarios.

[OliviaLynn]

Units: I do agree--but for now I did preserve the code Micheal wrote, feeling that refactoring it to be out of scope of the goal of this PR. I've recorded details for unit changes we want to make in this issue: #117

Check: In most cases, we shouldn't need this; however, if the user supplies transmission tables that overlap and do not share the same step (and the user specifies delta_wave=None), the values from each would be interleaved.

eg, (and going into this level of detail just to kind of think out loud/review this for myself here)
imagine a group with two passbands:
passband_a.waves = [2, 4, 6]
passband_b.waves = [5, 6, 7, 8]
which results in:
group.waves = [2, 4, 5, 6, 7, 8]
and if we just grabbed waves based on lower and upper bounds, we would get end up grabbing that extra "5" value when generating our indices:
[2, 4, 5, 6, 7, 8]
[1, 1, 1, 1, 0, 0] <-- mask representing the indices we get
when what we really want is:
[2, 4, 5, 6, 7, 8]
[1, 1, 0, 1, 0, 0] <--excluding the 5
It's a small edge case, but possible to encounter under the specs I've been given (my understanding from the last time I spoke to @hombit is that we would like to support non-uniform transmission tables, but I asked him a while ago and fairly informally, so it could be worth revisiting).
The check is at worse O(n), but if we can guarantee uniform table steps we could get rid of this entirely.

[mi-dai]

Thanks! That makes sense to me now. I think my confusion is does the check fails on one of the conditions or it only fails on both conditions (non-uniform grid AND overlapping). My understanding is the latter.

[mi-dai]

(but the doc implies the former?)

[mi-dai]

Sorry my review is late. After looking at the code, I would like to keep advocating for the following interface:

passband = Passband(delta_wave: float | None = 5, trim_percentile: float | None = 0.1)
band_flux = source.get_band_fluxes(passband, t)

This will avoid confusion from users having to call

spectral_flux = source.evaluate(passband.waves, t)

and having to know that the evaluate should be on the passband.waves grid.
And not having to check if user has actually call evaluate with the correct grid.