quatrope / astroalign

A tool to align astronomical images based on asterism matching
MIT License
142 stars 44 forks source link

Query regarding a section of the source code in `find_transform` #88

Closed prajwel closed 1 year ago

prajwel commented 1 year ago

I was going through the source code and could not understand the following code block in find_transform.


    inl_unique = set(tuple(pair) for pair in inl_arr)
    # In the next, multiple assignements to the same source point s are removed
    # We keep the pair (s, t) with the lowest reprojection error.
    inl_dict = {}
    for s_i, t_i in inl_unique:
        # calculate error
        s_vertex = source_controlp[s_i]
        t_vertex = target_controlp[t_i]
        t_vertex_pred = transform.matrix_transform(s_vertex, best_t.params)
        error = np.linalg.norm(t_vertex_pred - t_vertex)

        # if s_i not in dict, or if its error is smaller than previous error
        if s_i not in inl_dict or (error < inl_dict[s_i][1]):
            inl_dict[s_i] = (t_i, error)
    inl_arr_unique = np.array(
        [[s_i, t_i] for s_i, (t_i, e) in inl_dict.items()]
    )
    s, d = inl_arr_unique.T

Isn't inl_unique already having unique pair of (s, t)? What is the need for the remaining code?

martinberoiz commented 1 year ago

If I recall correctly (I'm at work now and can't see the code in detail) this is because even if the all the pairs are unique, you may have a situation like this (s1, t1), (s2, t1) in which there are two stars in the source (usually very close) that map to the same target t1. In that case we want to choose the pair whose source is closest to the target.

In practice, if two stars are very close in source there will be close in target too, and the algorithm may consider all of them probable. So you may end up having (s1, t1), (s2, t1), (s1, t2), (s2, t2) all possible mappings. That bit of code attempts to disambiguate it by choosing the pair with the lowest distance error.

prajwel commented 1 year ago

Thank you for your reply.

This selection is being carried out after the _ransac function has found the transformation. Shouldn't this disambiguation be part of the _ransac function?

martinberoiz commented 1 year ago

yes, I guess you can have that inside ransac, but in that case it would not strictly be the "random sample consensus" algorithm anymore. It would be a sort of modified ransac, specialized to this particular situation. I would like to keep ransac, as close as textbook as possible to swap it if a better implementation comes along (I believe skimage has one too.)

prajwel commented 1 year ago

Thank you, I will close this issue now.

I have been experimenting with some modifications to find_transform for my own use cases. Thank you again for your replies. Astroalign is extremely useful software.