lincc-frameworks / tdastro

MIT License
4 stars 0 forks source link

Passband optimization #112

Closed OliviaLynn closed 1 month ago

OliviaLynn commented 1 month ago

When calling fluxes_to_bandfluxes, we recalculate the portion of PassbandGroup.waves that overlaps with each individual Passband's waves. This could be done ahead of time and stored to make fluxes_to_bandfluxes faster (and recalculated each time the transmission table is re-gridded).

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.

_Originally posted by @jeremykubica in https://github.com/lincc-frameworks/tdastro/pull/111#discussion_r1763146109_

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.

_Originally posted by @jeremykubica in https://github.com/lincc-frameworks/tdastro/pull/111#discussion_r1763147523_

Also, see this comment explaining the check just following the calculation (if np.array_equal(self.waves[lower_index : upper_index + 1], passband.waves):), quoted here:

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, 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 Kostya 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.