Incorporating LyA 1B changes into AMTL by LNapolitano · Pull Request #162 · desihub/LSS

LNapolitano · 2025-07-29T21:04:29Z

This PR will contain the changes necessary to incorporate the LyA 1B numobs changes into the alt MTLs. Planning to add a new actiontype to the tiletracker file, that when encountered will run desitarget.mtl.update_lya_1b on the alt ledger directory.

… timestamp specification.

…in_tiles() calls

…1b tile.

…rk1b/bright1b

…es on 20250721

…AML file based on the file creation times of the real mtl ledgers

…ion such that it's per realization.

…w" actions to create new ledgers in an already existing set of alternate MTL ledgers.

…ew log print out

Removing special handling for tiles with 4.0 < FAVER < 5.0. Testing shows this is no longer necessary.

This should altmtltools to be used in pre desitarget version 3.4.0 environments, as is currently desirable for the mock alt-mtls due to memory issues in later versions.

…nality.

…add_new_ledgers affected).

comment out target option since is not used for mocks

Remove target option, is not used

remove targfile. Is not needed in mocks

remove targfile for mocks. Is not used

remove targfile option, is not needed

remove targfile option, is not needed for mocks

jalasker · 2026-06-24T14:54:50Z

Hi Lucas,

Just finished reviewing the pull request. Notes/Comments/Questions below.

Most of the comments are for improvements that aren't going to directly impact the function of the code, but I will highlight the comments for altmtltools.py L787 and fatools.py L388 as potential issues (though much more likely I didn't trace the logic exactly right).

The addnew function is somewhat unique in that it requires some variables set at the creation of a set of alt mtl ledgers, namely {"seed", "reproducing", "shuffleSubpriorities", "shuffleBrightPriorities", "shuffleELGPriorities", "PromoteFracBGSFaint", "PromoteFracELG"}. Presently the code looks to find these in an initialization log file (default format: "Initialize{Obscon}AltMTLsParallelOutput_mainRepro.out"). We will need to discuss how to best handle accessing these keywords for the mock altMTLs.

This is one of the reasons I liked having the script copied over from the initialization step. We don’t currently have any easily readable memory for this beyond the initial file (which, even with the copy checking step, can still be manually changed if it’s changed in the AMTL directory, so not perfect!). However, I believe we can store all of this information as metadata in the tile tracker ECSV header.

Comments on code changes:
bin/DA2ALTMTLRealizationsBRIGHT_mock.sh‎

L225 - Let’s remove the targfile variable from the code entirely at some point since only the mocks should have required this. Then we can remove the option and the commented associated options from the script files.
Not going to make this comment for every bin/*.sh file but it applies to all of them

bin/SingleNode-InitializeAltMTLsParallel.py

L94-96 - Can we safely remove the legacy code and those comments now? It sounds like YAML is working and we always have prior tagged versions on GitHub in case we need to access it again.
L159-169 - put these inside of an “if verbose and/or debug” block. It’s already hard to read the log files.

py/LSS/SV3/altmtltools.py

L35-43 - It should be more reliable to directly check the version of desitarget rather than excepting all ImportErrors
L178 (previously 166) - IIRC, the purpose of this check was to ensure that the argument bit2Flip was a power of two. If you want to remove the “int()” from around the second argument of that assert statement, you should replace the entire argument with “bit2Flip == bit2Flip” since you’re only catching nans with that anyway.
L515-546 - Anything related to parsing/creating the YAML files I’m going to assume is correct based on your testing since I’m not super familiar with the syntax. I want to get this review done quickly so you can start running the new AMTLs again. Let me know if you want me to review the YAML changes more closely.
L765-6 - This warning should be propagated to the bin/*.sh files and/or written to log file/console if set to False.
L787 - Shouldn’t one of these date checks be “<=“ or “>=“?
L837-885 - Flagging that we decided yesterday that we wanted to rewrite this without using svn directly in the script. A file written to a (DESI-)universally accessible directory in CFS sounded like a good option.
L926, 1035,1236,1238 - These, and many other log statements I left in this function, should be behind “if verbose” and/or “if debug” to clean up log files.

py/LSS/SV3/fatools.py

L388 - I’m a little worried about this line being applied to higher target versions than the default (1.1.1). These may(/will?) have new targets that aren’t present in 1.1.1 and so removing the check we have to see if v3.0.0 and 3.2.0 are being incorporated correctly feels suboptimal.

jalasker

Accidentally submitted the review as a comment, resubmitted all of the content from the comment as review comments directly on the relevant lines of code except for this comment in reply to one of your comments:

The addnew function is somewhat unique in that it requires some variables set at the creation of a set of alt mtl ledgers, namely {"seed", "reproducing", "shuffleSubpriorities", "shuffleBrightPriorities", "shuffleELGPriorities", "PromoteFracBGSFaint", "PromoteFracELG"}. Presently the code looks to find these in an initialization log file (default format: "Initialize{Obscon}AltMTLsParallelOutput_mainRepro.out"). We will need to discuss how to best handle accessing these keywords for the mock altMTLs.

This is one of the reasons I liked having the script copied over from the initialization step. We don’t currently have any easily readable memory for this beyond the initial file (which, even with the copy checking step, can still be manually changed if it’s changed in the AMTL directory, so not perfect!). However, I believe we can store all of this information as metadata in the tile tracker ECSV header.

jalasker · 2026-06-24T14:57:16Z

-#targfile='' #CHANGEME IF RUNNING ON MOCKS
-#targfile='--targfile=/global/cfs/cdirs/desi/target/catalogs/dr9/1.1.1/targets/main/resolve/' #Main survey target directory
+#ACR: we deactivate this option since is not used for mocks, it is not needed
+targfile='' 


Let’s remove the targfile variable from the code entirely at some point since only the mocks should have required this. Then we can remove the option and the commented associated options from the script files.

Not going to make this comment for every bin/*.sh file but it applies to all of them

Agreed. I'll pass this along to @aureliocarnero as well for the mock scripts. Not urgent, but good to remove the targfile variables at some point.

jalasker · 2026-06-24T14:57:39Z

-    log.debug('First healpixel: {0:d}'.format(HPList[0]))
-    log.debug('Last healpixel: {0:d}'.format(HPList[-1]))
-    log.debug('Number of healpixels: {0:d}'.format(int(len(HPList))))
+# 20260401 LGN - Modifying HPList to be read from a YAML file, not a csv


Can we safely remove the legacy code and those comments now? It sounds like YAML is working and we always have prior tagged versions on GitHub in case we need to access it again.

Thanks James! Agreed, we can remove some of the comments here. I think the HPList info is still useful, as we do load that from the yaml file. Also I've just rewritten this section to use a --yamlpath argument to specify the directory name, with the default value supplied.

28924ce

jalasker · 2026-06-24T14:57:55Z

            log.info(args.finalDir)
-        log.info('shuffleSubpriorities: {0}'.format(args.shuffleSubpriorities))
+
+        # 20260420 LGN - Adding some additional log statements here for reproducibility


put these inside of an “if verbose and/or debug” block. It’s already hard to read the log files.

Done! dd5349a

I just had to work out the tiletracker meta change first, as I was relying on these statements otherwise.

jalasker · 2026-06-24T14:58:20Z


+#LGN 20260617 - Adding a T/E wrapping around the update_lya_1b import
+#             - This should enable the this code to be used with older desitarget versions.
+try:


It should be more reliable to directly check the version of desitarget rather than excepting all ImportErrors

Agreed, the try/except was a little nasty. I've replaced this with a "hasattr" check:
95576c2

This whole block is hopefully temporary as the mock team investigates the memory issues they encounter in desitarget versions after 3.0.0

jalasker · 2026-06-24T14:58:40Z

 def flipBit(cat, bit2Flip, cond = None, fieldName = 'DESI_TARGET', mode = 'on'):
    #only works on single bits
-    assert( np.abs( np.log2(int(bit2Flip)) - int(np.log2(int(bit2Flip))) ) < 0.001  )
+    assert( np.abs( np.log2(int(bit2Flip)) - np.log2(int(bit2Flip)) ) < 0.001  )


IIRC, the purpose of this check was to ensure that the argument bit2Flip was a power of two. If you want to remove the “int()” from around the second argument of that assert statement, you should replace the entire argument with “bit2Flip == bit2Flip” since you’re only catching nans with that anyway.

Yeah, this is something that I believe broke with a new numpy version at some point? The log2 function required the explicit casting I believe. Anyway, since this is working for now I'm going to leave it as is, but I agree I can take another look later.

If the check here was solely for bit2Flip being a power of 2, then this answer is a lot smoother than what I wrote trying to avoid floating point rounding errors and vagaries of built-ins: https://stackoverflow.com/a/57025941 ((n & (n-1) == 0) and n != 0)

jalasker · 2026-06-24T14:59:57Z

    fn = initMTL.split('/')[-1]
    log.info('reading initial MTL(s)')
    allentries = Table.read(initMTL) 
+    log.info('initial MTL size: {}'.format(len(allentries)))


This, and many other log statements I left in this function, should be behind “if verbose” and/or “if debug” to clean up log files.

Added: 669b5e6

jalasker · 2026-06-24T15:00:04Z

            #makeTileTracker(outputMTLDir, survey = survey, obscon = obscon,overwrite = False,
            #startDate = startDateShort, endDate = endDateShort)
+        else:
+            log.info('tiletracker already exists, not overwriting')


“if verbose” and/or “if debug”

Added: 669b5e6

jalasker · 2026-06-24T15:00:08Z

            #JL - reset TARGET_STATES based on new target bits. This step isn't necessary for AMTL function but makes debugging using target states vastly easier. 
            initialentries['TARGET_STATE'][ELGNewHIP & np.invert(QSOs)] = np.broadcast_to(np.array(['ELG_HIP|UNOBS']), np.sum(ELGNewHIP & np.invert(QSOs)  ) )

+        log.info('Initial Entries Size: {}'.format(len(initialentries)))


“if verbose” and/or “if debug”

Added: 669b5e6

jalasker · 2026-06-24T15:00:13Z


+        log.info('Initial Entries Size: {}'.format(len(initialentries)))
        retval = desitarget.io.write_mtl(outputMTLDir, initialentries, survey=survey, obscon=obscon, extra=meta, nsidefile=meta['FILENSID'], hpxlist = [meta['FILEHPX']])
+        log.info('write_mtl return value: {}'.format(retval))


“if verbose” and/or “if debug”

Added: 669b5e6

jalasker · 2026-06-24T15:00:30Z

-                    log.info(f'reproducing = {reproducing}') 
-
+            #LGN 01/28/26: Adding compatibility with BRIGHT1B
+            elif targver == '1.0.0' or targver == '3.0.0' or targver == '3.2.0':


I’m a little worried about this line being applied to higher target versions than the default (1.1.1). These may(/will?) have new targets that aren’t present in 1.1.1 and so removing the check we have to see if v3.0.0 and 3.2.0 are being incorporated correctly feels suboptimal.

Yeah, I was admittedly a little unsure about this one. The intention here was to handle the very early survey tiles by directly copying their targ files rather than generating them via altcreate_mtl() is that correct?

Since this only ever runs with reproduction = True, it won't affect our standard reproduction = False runs. And I have verified that we can run dark1b/bright1b with reproduction = False. Maybe the best solution is to just remove 3.0.0 and 3.2.0 from this check and move them into the earlier statement:
if (not reproducing) or (targver == '1.1.1'):

But I'm not sure I totally understand the intention here either. Let me know what you think would be best.

The point I wanted to make here is related to the original reason this copy block was made for the reproduction runs.

Originally there was a very short period of time (few days-weeks?) where DESI used target version 1.0.0 rather than 1.1.1. It proved to be challenging to exactly handle this target version change with the precision required for an exact reproduction test, but it was simple (IIRC we didn't actually have to make any changes) to get it working good enough from a statistical POV given the small number of tiles observed in that period of time.

Therefore, we decided we could directly copy a handful of tiles and test that everything downstream of those tiles ran as expected and that would validate the remainder of the survey running on the same target version.

With the version change going forward into BRIGHT1B, we have a similar scenario where 1.1.1 was used for a period of time and going forward we are using 3.0.0/3.2.0. Except this time we are doing the exact target file copy for the new version which will be used going forward. This means that any reproduction test should automatically succeed starting at the date where you use 3.0.0/3.2.0. At best, it means that no targets in v3.0.0 /3.2.0 are being properly tested, but the v1.1.1 targets which are still remaining get properly tested.

… do not need to be built for each realization.

…Removing legacy comments.

… versions with a "hasattr" check instead.

… tiletracker for DARK surveys.

…oducing = True and TARGVER = 1.0.0) This ensures target files are generated using altcreate_mtl for DARK1B and BRIGHT1B fa actions

…the fiberassign header info for main survey tiles.

LNapolitano added 30 commits July 29, 2025 14:02

Starting developement on incorporating LyA 1B changes into AMTL

b1a7a7d

Adding Handling for Lya1B Actiontype in loop_alt_ledger

d7334b4

Adding tiletracker updating for lya1b action

477c09f

Ensuring LyA1B action is only run for obscon = dark ledgers.

cb69e6b

Adding optional lya1b flag for lya qso numobs increase action. Fixing…

920a178

… timestamp specification.

Adding handling for extension in desitarget.mtl.update_ledger calls

532e84a

Adding Handling for maketwostyle kwarg in desitarget.io.read_targets_…

e0b0c64

…in_tiles() calls

Adding additional update_ledget call to update dark ledgers on a dark…

087ef1e

…1b tile.

Adding reading and merging of dark and dark1b targets for dark1b tiles.

3381678

Modifying use of read_targets_in_tiles to pass two directories for da…

9e8d61f

…rk1b/bright1b

Reverting a small change that broke file path setup for reproducing runs

67ff6d8

Fixing dark1b/bright1b file specification for update actions.

d266a77

Changes to update_alt_ledger for 1b tiles observed before lya1b chang…

6b50b3f

…es on 20250721

syncing tiletracker update behavior with main branch

5074538

Temporarily removing desitarget version checks, no longer necessary

2131a61

Updating file permissions to match main branch

2552dad

Managing whitespace merge issue

7912472

Managing whitespace merge issue

1630005

Removing desitarget_version specification and associated code

f34cd6d

Adjusting update_ledger logic for 1B programs

3f7de3b

Updated to reflect make/update tiletracker changes.

330d330

Adding SingleNodeInitialize/SingleNoderun scripts from main

3ddef7f

Modifying HPList to load from yaml file, rather than csv

b8067d9

Initial Add of yaml file actiontype, including as tiletracker action

565c8b1

Added initializeYAML_HPTracker function, which creates a per-obscon Y…

4ec8f39

…AML file based on the file creation times of the real mtl ledgers

Adding new yaml file generation step to the SingleNode Initialize script

dbe7966

Initial add of YAML file check/regeneration in updateTileTracker

0c66d40

Adding some additional log statements, and moving the YAML file creat…

af08128

…ion such that it's per realization.

Initial commit of new "add_new_ledgers" function which handles "addne…

67f33be

…w" actions to create new ledgers in an already existing set of alternate MTL ledgers.

Fixing type casting in initializeAlternateMTLs call and adding some n…

95a2a3a

…ew log print out

LNapolitano requested review from ashleyjross and zjdingastro June 15, 2026 16:37

LNapolitano self-assigned this Jun 15, 2026

LNapolitano and others added 12 commits June 17, 2026 12:14

Update fatools.py to match main branch FAVER logic

5cc97b1

Removing special handling for tiles with 4.0 < FAVER < 5.0. Testing shows this is no longer necessary.

Adding a try/except wrapper to importing desitarget.mtl.update_lya_1b

946ed02

This should altmtltools to be used in pre desitarget version 3.4.0 environments, as is currently desirable for the mock alt-mtls due to memory issues in later versions.

Merge branch 'main' into LGN-LyA-1B

54384a0

Removing targets warning when running mocks. This is obselete functio…

8460e04

…nality.

Removing resolved TODO comment

843e892

Changing default YAML-TileTracker read location (makeTileTracker and …

498f1e6

…add_new_ledgers affected).

Update DA2ALTMTLRealizationsBRIGHT_mock.sh

f2e06ee

comment out target option since is not used for mocks

Update DA2ALTMTLRealizationsDARK_holi.sh

a0a6718

Remove target option, is not used

Update DA2ALTMTLRealizationsDARK_mock.sh

56a345d

remove targfile. Is not needed in mocks

Update Y1ALTMTLRealizationsBRIGHT_mock.sh

891c289

remove targfile for mocks. Is not used

Update Y1ALTMTLRealizationsDARK_mock.sh

db14485

remove targfile option, is not needed

Update Y1ALTMTLRealizationsDARK_mock_init.sh

18ea72a

remove targfile option, is not needed for mocks

aureliocarnero reviewed Jun 24, 2026

View reviewed changes

Comment thread bin/SingleNode-InitializeAltMTLsParallel.py

jalasker reviewed Jun 24, 2026

View reviewed changes

LNapolitano added 2 commits June 24, 2026 10:14

Removing initializeYAML_HPTracker() as files are now centralized, and…

140cc2f

… do not need to be built for each realization.

Adding new --yamlpath argument, to locate the per-obscon yaml files. …

28924ce

…Removing legacy comments.

aureliocarnero approved these changes Jun 24, 2026

View reviewed changes

LNapolitano added 9 commits June 24, 2026 14:58

Adding initialization keywords to tiletracker meta info.

5c5d26c

Adjusting add_new_ledgers to use the new tiletacker meta keywords.

70fed7d

Moving some log statements into an optional block (debug/verbose)

dd5349a

Replacing try/except wrapping for update_lya_1b in earlier desitarget…

95576c2

… versions with a "hasattr" check instead.

Adding informative log statements if lya1b action is not added to the…

5344865

… tiletracker for DARK surveys.

Adding >= to add_new_ledger tiletracker creation check

eb5aec7

Adding if (verbose or debug) checks to multiple log statements.

669b5e6

Changing get_fba_fromnewmtl to always call altcreate_mtl unless (repr…

35d85e6

…oducing = True and TARGVER = 1.0.0) This ensures target files are generated using altcreate_mtl for DARK1B and BRIGHT1B fa actions

Adding block to read the targver and target directory directory from …

1913965

…the fiberassign header info for main survey tiles.

Uh oh!

Conversation

LNapolitano commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jalasker commented Jun 24, 2026

Uh oh!

jalasker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LNapolitano commented Jul 29, 2025 •

edited

Loading