Fixes missing discard of duplicate secondary targets

[apcooper]

@geordie666 , I think this fixes the issue I raised by email: multiple rows in the output of select_secondary for (a) secondary targets that appear in more than one input catalog or (b) primary targets matched to more than one secondary.

Case (a) is fixed here:

desitarget/py/desitarget/secondary.py

Lines 648 to 654 in 203e392

	alldups = []
	for _, dups in duplicates(scxtargs['TARGETID']):
	dups = np.delete(dups, 0) # Retain the first
	alldups.append(dups)
	alldups = np.hstack(alldups)
	log.debug("Removing {} duplicate targetids".format(len(alldups)))
	scxtargs = np.delete(scxtargs,alldups)

Case (b) is fixed here:

desitarget/py/desitarget/secondary.py

Line 358 in 203e392

for _, dups in duplicates(primtargs['TARGETID']):

which was previously

desitarget/py/desitarget/secondary.py

Line 357 in 203e392

# for _, dups in duplicates(primids):

Question

desitarget/py/desitarget/secondary.py

Lines 547 to 554 in 203e392

	- Secondaries without `OVERRIDE` are also matched to themselves
	Such matches are given the same `TARGETID` (that of the primary
	if they match a primary) and the bitwise or of `SCND_TARGET` and
	`OBSCONDITIONS` bits across matches. The highest `PRIORITY_INIT`
	is retained, and others are set to -1. Only secondaries with
	priorities that are not -1 are written to the main file. If
	multiple matching secondary targets have the same (highest)
	priority, the first one encountered retains its `PRIORITY_INIT`

I didn't see the PRIORITY_INIT=-1 filtering implemented anywhere and it's not how I've handled case (a) in this PR. Wondering if I missed something about the intention here?

[geordie666]

I'll try to look at this closely in the next day. But, the filtering on PRIORITY_INIT of -1 is done in the write-to-file, here:

https://github.com/desihub/desitarget/blob/master/py/desitarget/io.py#L608-L615

Are you perhaps running the functions from within Python rather than running the command-line executables (such as select_secondary)? If so, could you see if your original issue persists when running select_secondary instead of calling the secondary.select_secondary() function directly?

[apcooper]

Ah, ok. Now I know where the filtering happens (I didn't check io.write_secondary) I'll just set PRIORITY_INIT=-1 rather than filtering in finalize_secondary.

[geordie666]

Thanks, again, for checking my working on this. I have a few requested changes before I merge...

[geordie666]

I don't think we want to retain the first duplicate among matching secondary targets, I think we want to retain the duplicate with the highest priority. Is it possible to rewrite this to retain the highest priority secondary-only target?

[apcooper]

Good catch, thanks -- will sort this out. Thanks for the review!

[apcooper]

Done, except I wasn't quite sure what was meant in the docstring by "the first encountered" . I wrote it to use the lowest index in the original table to break ties.

[geordie666]

Oh, "the first encountered" here just means that if we find two secondaries with the same priority, we just keep the first one in the table/array/loop. In other words, we don't break ties in any clever way once we've broken ties to keep the highest-priority secondary. That's not critical though. The way we write everything back to the input files and generate consistent TARGETIDs should keep track of what was matched and how.

If you don't think the code does this, just remove the sentence in the docstring. As I noted, it's not critical how we break ties other than on priority.

[apcooper]

OK, in that case I'll just use np.argmax directly, since that breaks ties on the lowest index in the list of duplicates. Done now.

[geordie666]

@apcooper: I think this looks good, now. If you agree I'll merge this PR.

Thanks for putting this effort into getting the secondary targets correct. It's important work and I'm swamped at the moment.

[apcooper]

OK, I pushed one last commit to simplify the tie-break part. Ready to merge now.

[geordie666]

One last request for good practice:

Can you add a line or two to the changes.rst file to describe this PR. Then I'll merge. Thanks!

[apcooper]

Done, hope that's enough info in changes.rst.

[geordie666]

Changes were good.