Avoid recalculation of phot_to and phot_to_params

[RainerHeintzmann]

Thank for the changes.
It would be nice, if recalculating phot_to and phot_to_params can be avoided by simply making them optional parameters in find_transform().
This line should only be executed under the condition ifnothing(phot_to).
This for aligning many frames to the same reference, this can significantly speed up the calculations as redoing the photometry for the reference image can be skipped.
Luckily these phot_to parameters are already returned.
It is actually eneought to just pass phot_to since the rest seems not to be used anyway.

[icweaver]

Good idea about re-using photometry if it's available. I think this can be handled by just adding a no-op to get_phot since we already have other dispatches there. Started a PR here that implements this generalization, comments welcome #54

[RainerHeintzmann]

I am not sure, if I understand.
You seem to have added functionality for aligning multiple images. This is essentially what AstroStacker does, but with many more options such as choosing the reference image, drizzle, and calculating error maps etc.
All I wanted was just this a few lines of change that thell AstroAlign to not recalculated reference photomety and in fac (I forgot) also the triangularization and sorting. I guess it would even be sufficient to be able supply the triangles only via kwargs, but this then should also trigger to skip all unnecessary calculations.

[icweaver]

Sure, that's great.

The aligning multiple images bit is really just a convenience function, not new functionality. The new part is just what you suggested here, i.e., re-using photometry if available, which the convenience function then naturally arises from

Re-using the triangularization and sorting can be investigated next

[RainerHeintzmann]

Hmm. Maybe I did not find the right commit, or it was the wrong PR? I did not see any change to find_transform(). Sorry, I am not used to revise PRs.

[icweaver]

The change is here:

get_phot(phot::Table; kwargs...) = phot, ()

which then propagates to find_transform

[RainerHeintzmann]

I see. It took me ages to understand what you are doing. I am not in favour of overloading the img_from argument.
to dispatch get_phot() into doing nothing sounds not very clean to me. and would also not prevent re-doing the (unnecessary) triangle calculations.
Finally this is actually some type of type-piracy since you are modifying the functionality of another function from a different package.

I would prefer changing line:

Astroalign.jl/src/warp.jl

Line 160 in 64e50df

phot_from, phot_from_params = get_phot(img_from; box_size, ap_radius, f, min_fwhm, nsigma, N_max, use_fitpos)

and line:

Astroalign.jl/src/warp.jl

Line 164 in 64e50df

C_from, ℳ_from = _triangle_invariants(phot_from)

and allow a named argument for the pre-calculated triangles (only), which would then automatically skip both of these steps indpendend on what is supplied for img_from.

[cgarling]

Making the triangles should not be so expensive that we have to save the triangles and reuse them; see #55 .

I like the method of overloading get_phot. Semantically it indicates that it will get photometry from the arguments; if the argument is already a photometry table it just returns it back.

[icweaver]

Thanks, Chris, agreed.

Rainer, just to clarify:

to dispatch get_phot() into doing nothing sounds not very clean to me.

This is a common multiple dispatch pattern which I tend to prefer over introducing an internal branch, as more commonly seen in Python programming.

and would also not prevent re-doing the (unnecessary) triangle calculations.

This seems like an orthogonal concern that Chris is addressing nicely in #55

Finally this is actually some type of type-piracy since you are modifying the functionality of another function from a different package.

No, get_phot is a helper function that we own within Astroalign.jl. It is defined in the same location that I linked to in my previous comment

For the reasons stated above, we are going to move forward with #54 and close this specific issue once it is merged. You are welcome to open future issues if you still see gaps in performance or functionality that have not been addressed here.

[RainerHeintzmann]

Yes, I agree. Since get_phot is defined in the package, this is fine. Sorry for my lag in understanding.
I now added this reuse-feature also to the AstroStacker.jl package. All that is missing is a release of Astroalign.jl to make it all smooth.