Patch 202210 #436
Patch 202210 #436
Conversation
…; and small re-write of the already existing file check
|
I forgot to mention that the core patching code has originally been written by @dstndstn (here https://github.com/desihub/fiberassign/blob/f163e9908da4f43505059f31e7b19e325c20c83a/bin/patch-fa-2.py), which I thank a lot for that! One technical python question for people more familiar with fitsio than I am:
In addition to being not elegant at all, I am a little worried that this line may change things in the format or that |
|
for info, I ve run few sanity checks (patched. vs. svn) on the headers of the patched files in I notice one required modification (I ll take care of it), and a few probably harmless differences. Required modification: in the current code, when copying the header, we remove the COMMENT keywords: fiberassign/py/fiberassign/fba_patch_io.py Lines 1424 to 1429 in 32f9181 that has to be changed, as in some early tiles, some information was stored in those COMMENT cards. for instance: /global/cfs/cdirs/desi/target/fiberassign/tiles/trunk/080/fiberassign-080611.fits.gz which are no more in /global/cfs/cdirs/desi/users/raichoor/fiberassign-patch/20220928-patch/080/fiberassign-080611.fits.gz. (Harmless) difference nº1: whereas those do not exist in the svn file: I am not sure what is the reason for that. (Harmless) difference nº2: whereas they are no more duplicated in the patched files; e.g.: This sounds also harmless; but could probably be fixed by the above required code change, which is about the COMMENT keywords. (Harmless) difference nº3: |
|
@sbailey : for info, I m re-generating a set of patching tiles with this fix for the COMMENT header (see commit just ~now). I ll try to perform few extra checks on my side but, as said, it s kind of limited as, by definition, I m using the same tools than for patching. I suggest that we go ahead and commit the changes this afternoon or tomorrow? For info, the breakdown of the 3891 patched files:
|
|
@araichoor I'm only seeing directories 000-011 and 020-024 in |
|
yes, please look at the only difference is for this COMMENT header thing for some tiles, so looking at thanks! |
|
for info, I ve run an independent check for the
for most of the targets, the check is to simply consider that those columns are consistent with what though, there are some "special" cases to deal with (and took 90% of the time to code...): (1) secondary-only or ToO targets should have BRICKID=BRICK_OBJID=0 (see discussion here #423). (2) some gaia-only targets should have a meaningful BRICKID, but OBJID=-1. ps: if useful, this file lists the found inconsistencies in the svn fiberassign files: |
|
and for sanity, this file contains the the remaining differences are: if the (1) and (2) cases are problematic, please let me know (and then I d appreciate hints of how to fix those). |
|
for info:
|
|
Sorry that I have been no help here... is there anything specific you would like me to check? PS - Anand is a hero! |
|
Okay, one quick comment. Files such as |
|
oh, very nice catch! fiberassign/py/fiberassign/fba_patch_io.py Line 1451 in 086cc93 I ll update the code (or you can go ahead if you re willing to :)) (if useful, note that there is this function fiberassign/py/fiberassign/fba_launch_io.py Lines 2058 to 2082 in 8e6e826 yes, any check on formatting would be great! |
|
I thought it was enough to use a filename of |
|
@dstndstn I m back here. fiberassign/py/fiberassign/fba_patch_io.py Line 1416 in 086cc93 ie could using suffix=".fits.gz" solve that?
I ll make some test-n-tries on one tile, and ping you for confirming things look good. |
|
This is what I have to do in fastspecfit to get gzipped output, but I think I just stole this from desispec somewhere-- |
|
@araichoor Yeah, that's what I was thinking, changing that suffix from .fits to .fits.gz . A quick test here shows that to work, hopefully it also works on the complicated multi-table fiberassign files! |
|
thanks @moustakas. I ve given a shot at the simple suggestion I made, i.e. use: as @dstndstn says, it looks like it should be working. I ll make that change. I ll wait for ~tonight to re-generate a new set of files, in case we discover other things today. |
|
I've done a wide range of checks on these files---including looking at the diagnostic files which @araichoor has generated---and I haven't found any totally obvious problems. As a further check, I went ahead and generated the I found differences with 44,164 objects, all from either with and |
|
Focusing on the subset of SV1 secondary targets, which have historically been problematic, it looks like my original ("old") catalogs had a bug or some other issue possibly related to the current, non-patched fiberassign files for ~25k of these objects. (Another 1.5k objects are only different in some of the Gaia photometric quantities, which were patched, so that all looks good.) Here's a snapshot of those objects: and |
|
for info, I ve run another "independent" check on the patched files: if I exclude the EBV and FLUX_IVAR_W{1,2} modifications, the modified rows mostly are for details: Then remain only modifications for tiles:
For those early tiles from Dec. 2020, the rows are modified for ~all the TARGETIDs, because we patch the fact that some columns were not propagated then, and just filled with zeros (BRICKNAME,HPXPIXEL and BRICKID,BRICKNAME,BRICK_OBJID,HPXPIXEL,RELEASE). So it overall makes sense (to me at least!). |
|
and a last check report, on the gaia column values consistency with here, what I do is:
all discrepant values happen for TARGETID where the patching nulled the REF_ID (and the mask is SCND_ANY), i.e. the target is not officially drawn from gaia, it comes from a secondary-only program or a ToO, and have no REF_ID. while doing that check, I also uncover an apparent issue in Tractor, ie in the ls-dr9 files. |
|
I agree that the minimalist comment is sufficient, though let's merge + run "python setup.py version" to update the dev version that gets recorded before the final final rerun on the files so that the fiberassign version matches what was actually used. |
|
ok perfect, let s go for the minimalist comment. about: run "python setup.py version" to update the dev version shall I do the following?
|
|
Bummer about the ls-dr9 gaia targets, but this PR and cross checks look good, so merging now. Thanks. And yes about steps for updating the dev version number. If you hit any problems with that, ping me on slack and we'll sort it out. |
|
@sbailey : there were two commits I haven t pushed yet ! (sorry I realize my previous message was ambiguous). then I ll push my two commits + merge. |
|
I restored the branch, but I'll ping you on slack to coordinate next steps |
|
for the record: I ve uploaded on DocDB the preparatory document used to make the diagnosis of the inconsistencies: https://desi.lbl.gov/DocDB/cgi-bin/private/ShowDocument?docid=7086. |
|
Is it still possible to add: To HDU 0 of the patched files? |
|
I finished the svn commit (nersc) + checkout (kpno) on Sunday. in theory, I think I could re-generate the files with adding that if I understand correctly: that would be to handle in a cleaner way the added |
|
Adding If we're already at the svn commit stage though, it's probably too late to add it. We can still run |
|
Also |
|
I didn t run fitsverify; I did run some checks against the original files. |
|
I ve run 52 are I think the e.g. running this command: |
|
I believe you that most of the warnings are due to missing In particular, let's assume that for some reason every HDU in a particular file needed to have the Where does the extra warning come from? So if nothing else, there is still something suspicious going on. I would like to actually see the examples that have 6-7 warnings. |
|
isn t this command from my last message demonstrating it s only due to missing about the
there are 1x8=8 then one legitimate question is: why for those dithers tiles, all extensions get a for non-dithered tiles, the dependencies are stored differently: that s likely explainable with digging into |
|
for completeness, here is one example of a |
|
I'll take a look myself, but |
|
ok thanks! besides, I ve on my to-do-list to update the fiberassign datamodel with a comment that some dithered tiles can have a 7th extension: https://desidatamodel.readthedocs.io/en/latest/DESI_TARGET/fiberassign/tiles/TILES_VERSION/TILEXX/fiberassign-TILEID.html. |
|
OK, I have confirmed your findings. It is indeed possible for every HDU to contain a Since this PR is already merged, I think we can just stop commenting to call this done. Thanx for the extra QA. |
This PR prepares the Fall 2022 patching (dubbed "patch_202210") of the fiberassign files.
Patched files (in
$DESI_ROOT/users/raichoor/fiberassign-patch/20220928-patch) still need to be vetted, but I am opening the PR as Work In Progress.The first patching was done on Oct. 5, 2021.
This PR should addresses multiple raised issues (e.g., #375. #403, #423, #432, #433).
Patching approach:
The patching philosophy is:
In addition, we agreed to do in the FIBERASSIGN-extension:
The discrepancies can arise from:
We agreed to let some remaining discrepancies, and just document them.
For instance, few REF_EPOCH values (for PMRA=PMDEC=0 objects), some MWS_TARGET for "specialbackup" test tiles designed with preliminary desitarget catalogs, some early tiles with extra columns, etc.
Method:
There are three scripts:
py/fiberassign/fba_patch_io.py: utility script with all relevant functions;bin/fba_patch_diagnosis: execute the diagnosis;bin/fba_patch: execute the patching, with writing patched files in a separate folder (and save all changes in a fiberassign-TILEID-patching_202210.ecsv file).And one file:
py/fiberassign/data/patching_202210.yaml: lists the desired patched columns, added columns, and populate_ebv behavior.Test files:
My current test files are in:
$DESI_ROOT/users/raichoor/fiberassign-patch:20220928-diagnosis: the diagnosis on the svn fiberassign files;20220928-patch: the patched files;20220928-patch-diagnosis: sanity check, diagnosis run on the patched files;20220928-patch-patch: sanity check, patching run on the patched files.The sanity check have "expected" results with:
20220928-patch-diagnosis:20220928-patch-patch: no remaining patching to do on the patched files.Of course, those tests are kind of circular, so extra-checks are required!