DM-10453: Fix bugs in matchPessimisticB #69
Conversation
timj
changed the title
timj
changed the title
| @@ -451,22 +452,23 @@ def _doMatch(self, refCat, sourceCat, wcs, refFluxField, numUsableSources, | |||
| pattern_skip_array=np.array( | |||
| match_tolerance.failedPatternList) | |||
| ) | |||
| if len(matcher_struct.matches) < 0 and \ | |||
There was a problem hiding this comment.
Sorry I let that through earlier.
| try_idx == 0: | ||
| if try_idx == 0 and \ | ||
| len(matcher_struct.matches) == 0 and \ | ||
| match_tolerance.lastMatchedPattern is not None: | ||
| # If we found a pattern on a previous run and can't | ||
| # find an optimistic match with the harshest tolerances, | ||
| # the match we found previously was likely bad. We append | ||
| # its index to a list so we do not use it in future match | ||
| # iterations. |
There was a problem hiding this comment.
These comments sound really helpful but I confess I don't see how they relate to the tests above. Can you think of some way to rephrase them a bit that would help make the connection clearer?
There was a problem hiding this comment.
Let me know if these comments are better.
| @@ -123,11 +123,11 @@ class we use id to reference the position in the input reference | |||
| self._pair_id_array[ref_id, ref_id:] = sub_id_array[:, 1] | |||
| self._pair_delta_array[ref_id, ref_id:, :] = sub_delta_array | |||
| self._pair_dist_array[ref_id, ref_id:] = sub_dist_array | |||
|
|
|||
| # Don't fill the array column wise if we are on the last object | |||
| # to avoid array overrun. | |||
There was a problem hiding this comment.
Could you clarify this comment? I think you fill the array if we are not on the last object, but if you are on the last object then you don't fill the array at all. If that's really what you are doing then something like this might be clearer:
# If not the last object, append ?? to the _pair arrays. Otherwise do...??? to avoid array overrun
| output_spokes.ref_spoke_list = ref_spoke_list | ||
| output_spokes.src_spoke_list = src_spoke_list | ||
| return output_spokes | ||
|
|
||
| return output_spokes | ||
|
|
||
| # TODO: Possibly clean up the arguments here by pre computing |
There was a problem hiding this comment.
I realize this is pre-existing code but...can you please file a ticket for this cleanup and reference the ticket in the TODO code comment? That's how we are handling TODOs these days in order to keep track of them.
There was a problem hiding this comment.
This was left over when when I was thinking about changing the function inputs. @ctslater and I had a look and tried a marginally simpler version but we both decided it wasn't really any better. I have thus removed the TODO statement.
| # tolerances as found in the previous match-fit, | ||
| # the match we found in the last interation was likely bad. We | ||
| # append the bad match's index to the a list of | ||
| # patterns/matches to skip on subsiquent iterations. |
There was a problem hiding this comment.
That's much clearer. Thanks! Two minor typos: "interation" -> "iteration" (twice) and "subsiquent" -> "subsequent".
The major bug fixed in this commit involves how the lookup table _pair_id_array in pessimistic_pattern_matcher_b_3D.py was filled. Previous to this commit the vertical slices of this matrix (line 130) was incorrectly filled with the second id of the reference pair. This mean that the lower half of the matrix contained incorret data and the last row slice (_pair_id_array[-1, :]) contained only one value of n_reference-1. This array is imporant as it is used to retrieve data needed to compare spokes in our pattern. This fix now fills the lower triangle of the matrix with the correct data for later lookup and comparison. matchPessimisticB.py: Fixed lastMatchedPatterna and failedPatternList bugs. Changed default from list type to None type. pessimistic_pattern_matcher_b_3D.py: Fixed pair_id data structure bug and changed output structs to not fill with partial results. Removed default empty string from MatchPessimisticTolerance. matchPessimisticB.py: Changed default input from list to None type. Fixed bug in pessimistic_pattern_matcher_b_3D.py Editted comments for clarity. Fixed typos in comment strings.
morriscb
force-pushed
the
tickets/DM-10453
branch
from
6497c50
to
a54e07b
Compare
The major bug fixed in this commit involves how the lookup table _pair_id_array in pessimistic_pattern_matcher_b_3D.py was filled. Previous to this commit the vertical slices of this matrix (line 130) was incorrectly filled with the second id of the reference pair. This mean that the lower half of the matrix contained incorret data and the last row slice (_pair_id_array[-1, :]) contained only one value of n_reference-1. This array is imporant as it is used to retrieve data needed to compare spokes in our pattern. This fix now fills the lower triangle of the matrix with the correct data for later lookup and comparison.
matchPessimisticB.py: Fixed lastMatchedPatterna and failedPatternList bugs. Changed default from list type to None type.
pessimistic_pattern_matcher_b_3D.py: Fixed pair_id data structure bug and changed output structs to not fill with partial results.
Removed default empty string from MatchPessimisticTolerance.
matchPessimisticB.py: Changed default input from list to None type.