if both columns of lastnight are present, use only one

[andreufont]

If LASTNIGHT is present, then just use this.

If not, check if either LAST_NIGHT or COADD_LASTNIGHT are present.

[abrodze]

This looks good. I left one comment on the priority of columns, but I am not sure it impacts the outcome in any way.

[andreufont]

@abrodze , @corentinravoux - Let's try to get this right, since this variable is used to decide the blinding (and if we make a mistake, we might accidentally not blind the data).

From what Paul mentioned on Slack:

• LASTNIGHT and COADD_LASTNIGHT are column names used in the official data products
• In tile-based data, LASTNIGHT should be an integer (last night of observation contributing to that tile)
• In healpix/uniqpix data, LASTNIGHT should be a string of integers (one per tile)
• In healpix/uniqpix data, COADD_LASTNIGHT is the largest of the (potentially several) values in LASTNIGHT

Based on that, we should:

• check if COADD_LASTNIGHT exist, and if so use that
• if COADD_LASTNIGHT does not exist, then see if LASTNIGHT exist and use it (this should only happen in tile-based catalogues)

What is LAST_NIGHT? Is this also a column in the DESI data model?

[andreufont]

We discussed this today at the telecon.

We agreed that we should have a safety net to prevent unblinding by mistake, and one way to do this is to enforce the column COADD_LASTNIGHT. This is already in the DR3 catalogs, and it leaves no room for mistake.

One could also allow for LASTNIGHT, since this would allow us to recycle the DR2 catalog. Alternatively, one could create a new version of the DR2 catalog with that extra column.

At any rate, we agreed to drop LAST_NIGHT since we haven't used it since EDR tests.

[andreufont]

Actually, the DR2 catalog already uses COADD_LASTNIGHT and LASTNIGHT is not present (good job DR2!), so I've decided to enforce COADD_LASTNIGHT or crash. This was already the preferred option yesterday at the telecon.

[andreufont]

@abrodze , @corentinravoux - I think we are ready to merge this now.

[abrodze]

The tests are failing on the QSO catalogs in /pscratch/sd/a/abrodze/dev/picca/py/picca/tests/delta_extraction/data which are fuji era catalogs.
Should we update these to a DR2 version or enforce backward compatibility through the beginning of the survey?

[corentinravoux]

Even if that is a pain, I suggest to have backward compatibility. Picca was meant to be multi-survey at first, I think it is important that we keep the compatibility for the whole DESI survey at least

[abrodze]

Backwards compatibility should be preserved now with the recent changes.
The code now checks for a COADD_LASTNIGHT or LASTNIGHT column, preference on the former, to determine if the data is DR2 or later. If yes, then COADD_LASTNIGHT is required else LASTNIGHT is accepted.

The main remaining failure point for the tests was the lack of night column in the mock catalogs. For now, I use a filler max_date=0 if there is no COADD_LASTNIGHT or LAST_NIGHT column (thus no blinding). This seems like a weak point for accidental unblinding, but someone else should take a look. It would probably also be good to replace the test QSO_cat from fuji with at least an iron version if not a loa version.

This PR is now not that much different from main aside from requiring COADD_LASTNIGHT if the data is > DR1 cutoff.

[corentinravoux]

Interesting point. My opinion is that picca should be able to work on main DESI releases (EDR, DR1, DR2, DR3) so I guess we should add separated tests for fuji, iron, loa and the release after matternhorn. I will add an issue on this point.
I am ok to push

[corentinravoux]

Approved after backward compatibility added

[corentinravoux]

I let @andreufont adress the max_date=0 point, if it is ok, we can push

[andreufont]

Thank you both, I think we can merge this!