search

desihub / desitarget

DESI Targeting
BSD 3-Clause "New" or "Revised" License
18 stars 23 forks source link

Override ledgers don't seem to be backwards-compatible with SV3 #784

Closed geordie666 closed 2 years ago

geordie666 commented 2 years ago

See the discussion in #767. In particular, the issue could be that the NUMOVERRIDE column didn't exist prior to the Main Survey.

jalasker commented 2 years ago

From io.py line 3066-3069 in find_mtl_file_format_from_header

 try:

    override = read_keyword_from_mtl_header(hpdirname, "OVERRIDE")

except KeyError:

    pass

override is a keyword argument that can be set to true in order to find mtl_file_format for override header rather than normal header.

However, if the header has a keyword "OVERRIDE" that is "false" the keyword argument "override" gets overwritten.

I think I may have put that override in the MTL meta data to try and prevent this exact issue from happening... I am trying to find out where that happened, but in the meantime, there should be some clarification as to how that override keyword is supposed to work, especially since other functions that get called within other functions call using both 'override = True' and 'override = False' and rely on the results being different when requesting the override header version.

The way I solved the issue is wrapping that above block of code in "if not override" so that it preserves the input of override = True, but allows the header to set override = True if the code leaves it at the default value of false.

jalasker commented 2 years ago

@geordie666 do you think this could be the actual issue I'm having with the SV3/override ledger issue? I don't think it can possibly be the desired behavior to override a user/script provided value of "override" based on something that may be in the header of a file.

geordie666 commented 2 years ago

@jalasker: I finally had a chance to check for backwards-compatibility with the SV3 ledgers, and I can't find a problem. For the record, here's what I did:

I find that everything processes without an error.

So, I think you must have introduced something into the alternate MTL ledgers that breaks the intentions of the code (as you're also suggesting).

I'm not sure I understand your use case, though. There are no override ledgers for SV3, so any SV3 analysis should proceed without any override filenames/directories/headers. Anything to do with "OVERRIDE" should just be missing completely from SV3.

To clarify the intended behavior of find_mtl_file_format_from_header():

I've tested this for an example SV3 file, and I believe that the behavior is as intended. Consider the file in:

/global/cscratch1/sd/adamyers/sv3mtl-dark-hp-6481.ecsv

I've tried the following use cases:

So, this all looks OK? Can you give an example of a file header that doesn't have my expected behavior?

I could certainly improve the docstring to indicate that this is the expected behavior (and can now reference this issue for futher clarification!).

jalasker commented 2 years ago

I think the disconnect between what I'm doing and your testing is that you are copying the originally generated MTLs for SV3 and I am making new MTLs using the latest version of write_mtl.

Anything to do with "OVERRIDE" should just be missing completely from SV3.

I agree with this. But the current version of write_mtl does not.

lines 828-829 include keys/vals to be added to the ecsv metadata or fits header:

keys = ["INDIR", "SURVEY", "OBSCON", "SCND", "OVERRIDE"] vals = [indir, survey, obscon, scnd, override]

And no matter what the survey is, the keyword "OVERRIDE" is being added.

What I did insert into the code was asserting that "OVERRIDE" is always false for my SV3 ledgers, but it shouldn't be present at all.

geordie666 commented 2 years ago

Right, so one solution is to fix this by not writing the OVERRIDE keyword at all if override=False in write_mtl(). Then newly written ledgers would resemble what we did for the original ledgers. That's my preferred solution, if that works for you? I could add that today.

But, just for my own understanding, why doesn't this case work:

My tests with a file that contains OVERRIDE: false in the header seem to suggest that find_mtl_file_format_from_header() works exactly as expected. If your alternative sv3 ledgers are written with OVERRIDE: false in the header, why doesn't find_mtl_file_format_from_header() just work automatically?

jalasker commented 2 years ago

find_mtl_file_format_from_header() works in the way you described it, but that is not the immediate problem.

In update_ledger (and several other places but this is where I am running into the issue), the code is assuming that passing override=True is going to return the override ledger format and (for me) breaks if the header info causes it to return the normal ledger.

lines 1811-12 of mtl.py:

# ADM this is the format for any associated override ledgers. overrideff = io.find_mtl_file_format_from_header(hpdirname, override=True)

If the ledger has override = False in the header, then it will return the regular ledger format for overrideff.

Then later down in the function, lines 1861-66

overfn = overrideff.format(pix)

    # ADM if an override ledger exists, update it and recover its
    # ADM relevant MTL entries.
    if os.path.exists(overfn):
        overmtl = process_overrides(overfn)

Then in process_overrides (still mtl.py, line 1275):

mtl["NUMOVERRIDE"] -= 1

Since the ledger that is passed to process_overrides is not an override ledger, this line fails since the non-override ledgers (at least in SV3) don't have the keyword NUMOVERRIDE.

Not adding "OVERRIDE: false" to the header should solve that issue since it won't cause overrideff to point to the regular ledger.

However, for robustness' sake, the code should just not run process_overrides if overrideff doesn't actually point to the override ledger. It could be in the form of "if 'override' in overfn" before calling process_overrides or something similar within process_overrides where you just return 0 or None if the check fails.

You can reproduce the failure by adding "OVERRIDE: false" to $CSCRATCH/blat/mtl/sv3/dark/sv3mtl-dark-hp-6477.ecsv and otherwise performing the same test you described above. Note that it is that file and not any of the files that are actually updated for tile 551 since "find_mtl_file_format_from_header" and ultimately "read_keyword_from_header" are only given a directory, not a filename as arguments and determine which file's header to read by glob, and (at least my own version of) glob finds that file first.

geordie666 commented 2 years ago

Thanks for the detailed explanation, which helped a lot. I believe I have a backwards-compatible fix, and I'll push a PR later today for you to test against.

geordie666 commented 2 years ago

Addressed in #789.