Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Effective exposure times of backup program #2370

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Conversation

segasai
Copy link
Contributor

@segasai segasai commented Sep 11, 2024

Following recent discussion on slack @schlafly

This is a PR (in development) addressing some issues with the effective exposure times of backup program

At the moment that just does removes the dust extinction correction when computing the tsnr2 for gbbackup/backup tracers.

I would appreciate here any suggestions on the easiest way to test the effect of changes of tsnr.py on individual exposures without running the full pipeline. That would help understand the effect of changes.

@weaverba137
Copy link
Member

I recommend taking another look at PR #1815 as part of this.

@segasai
Copy link
Contributor Author

segasai commented Sep 11, 2024

Yes, I thanks, I was aware of the other PR, but since it was a one-liner PR (like this one ATM), I decided to open a separate one. But I agree this should supersede #1815.

Following up on specific 1815 discussions

From my understanding when we update the nominal flux we need to rerun the:
https://github.com/desihub/desispec/blob/37e454d8ace16b6337a04bac8b8e58ed2646496d/py/desispec/scripts/calibrate_tsnr_ensemble.py

(i.e. #1274 and
https://desi.lbl.gov/svn/code/desimodel/trunk/data/tsnr/README
)

@araichoor
Copy link
Contributor

I ve to carefully read the -- long and rich! -- slack thread about that, so sorry if I m adding noise here.

but from what I understood from skimming it, the goal is to have the backup tiles observed to a given efftime_spec, with discarding the ebv factor, right?

if so, I kind of feel that it may be better to tackle that at the per-{exposure,tile} information, rather than at the per-fiber, no?
i.e. to remove the ebv factor in the per-{exposure,tile} efftime_spec for backup tile.
also, changing it at the per-spectrum level will create a change for mainbackup fibers only after this pr, which could make people using the data tripped on that; changing it at the per-tile level is much less "risky" I feel, as it would mostly affect operations only - and some tile selection based on efftime_spec.

one would have to double-check, but I think that the offline efftime_spec info used by nts is the per-tile one.
this per-tile info comes from the tile_qa.py script, here I think:

# tile EFFTIME is median of efftime of fibers that are good in all exposures
always_good_fibers = ((or_qafiberstatus&bad_fibers_mask)==0)
if np.any(always_good_fibers):
tile_efftime = np.median(tile_fiberqa_table["EFFTIME_SPEC"][always_good_fibers])
else:
tile_efftime = 0.0

and it is a combination of the information in per-exposure level:

# fiber EFFTIME is only counted for good data (per exposure and fiber)
tile_fiberqa_table["EFFTIME_SPEC"]=np.zeros(targetids.size,dtype=exposure_fiberqa_tables["EFFTIME_SPEC"].dtype)
for i,tid in enumerate(targetids) :
jj = (exposure_fiberqa_tables["TARGETID"]==tid)
tile_fiberqa_table["EFFTIME_SPEC"][i] = np.sum(exposure_fiberqa_tables['EFFTIME_SPEC'][jj]*((exposure_fiberqa_tables['QAFIBERSTATUS'][jj]&bad_fibers_mask)==0))

the script for the per-exposure if exposure_qa.py.

what I suggest would be to add few lines in tile_qa.py - and possibly the same in exposure_qa.py, after the per-{exposure,tile} computation, saying "if (main?) backup tile, then remove (or add?) the ebv factor".

would that make sense?

@segasai
Copy link
Contributor Author

segasai commented Sep 11, 2024

Thanks for the comment @araichoor !
One of the goals is indeed to remove the ebv factor, but another is to make sure that the TSNR2 are computed for brighter 'templates'. (there could be other things there as well, as I may not have a full understanding of the issue)

I personally don't have an opinion on 'riskiness' of the change as I am less familiar with operational side of things. From user standpoint, it obviously would be good if the TSNR2/EFFTIMEs for individual fibers were consistent with what is done when planning the observations, but maybe that's not really achievable, given that we are potentially planning to make the change in the calculation of those for backup, so there will be always inconsistency for past vs new observations.

@deisenstein
Copy link

deisenstein commented Sep 11, 2024 via email

@schlafly
Copy link
Contributor

I'm really embarrassed that I don't know what gpbbackup is. When do we set that? I'm not aware of any gpbbackup tiles in the tiles file. Reflecting a little more, maybe that's "GFA pass band" and is used for the offline EFFTIME estimates that are most turned to match what we get from the GFAs (i.e., for checking the results from the exposure time calculator).

I think @sbailey or @akremin would be the best references on how to rerun the tsnr code for a handful of files to see what things look like.

I think I would be in favor of changing it at the per-fiber level, and keeping the tile derived quantities as medians of the fiber derived quantities. The biggest issue with that approach would be that we would have some cases where the same target is observed in the backup and bright tiles, and has apparently lower EFFTIME in the bright tile and higher EFFTIME in the backup tile, but only because the EFFTIMEs mean something different. But I think that's okay because we are explicitly saying we want the EFFTIMEs to mean something different in these cases!

I also think that you don't need to make "brighter templates"---IIRC the templates just care about the wavelength dependence and not the magnitude---and instead the earlier PR should be doing enough there, albeit after the calibration from TSNR2 to EFFTIME is changed.

After we merge something like this we will definitely have a time in daily when past backup EFFTIMEs mean something different from new backup EFFTIMEs. We could consider updating the past backup EFFTIMEs, or just move forward and update them as part of the next DR.

Daniel, I think I agree with everything you say, but I think all of the machinery is there to support what you want, and it's just a matter of picking the right source flux in the EFFTIME calculation---e.g., 18th mag, or 16th. I agree with you that this ends up being a trade with respect to how much effective time we get for the faint vs. the bright stars in different conditions, and that we just have to accept that there will be a trade there.

I also agree that in principle we could ~give up on the ETC and just accept that it will observe each backup tile to ~600 s and then get the EFFTIMEs right only in the offline pipeline. It would be better to keep both in sync, though.

@schlafly
Copy link
Contributor

@julienguy thinks that it should be straightforward to generate TSNR2s for representative data; pinging him here. He also knows about the TSNR2 <-> EFFTIME calibration.

@deisenstein
Copy link

deisenstein commented Sep 20, 2024 via email

@deisenstein
Copy link

Adding the figures here:

Here are the G=16 figs:

Backup_snr16_exptime.pdf

Backup_snr16_hist.pdf

Comparison to G=17.75

Backup_snr16_snr18.pdf

Here are the G=17.75 figs:

Backup_snr18_exptime.pdf

Backup_snr18_hist.pdf

Here is G=17.75 for the cases where we have only one exposure

Backup_snr18_exptime_nexp1.pdf

Backup_snr18_hist_nexp1.pdf

And here's the Bright data at G=18.75.

Bright_snr19_hist.pdf

@deisenstein
Copy link

Trying again, sorry:

G=16 plots:

Backup_snr16_exptime

Backup_snr16_hist

G=17.75 plots:

Backup_snr18_exptime

Backup_snr18_hist

Comparison of the two:

Backup_snr16_snr18

G=17.75, limited to tiles with only one exposure/visit:

Backup_snr18_hist_nexp1

Backup_snr18_exptime_nexp1

MWS G=18.75:

Bright_snr19_hist

@segasai
Copy link
Contributor Author

segasai commented Sep 20, 2024

Thank you Daniel!

Just a followup/extension from what you've done. I've taken first exposure (cframe) of backup tiles that are repeated vs are considered done after first exposure.
Here's how the SN(R) depends on magnitude for both:

image

The notebook used to produce the plots is on NERSC here:
https://data.desi.lbl.gov/desi/users/koposov/backup_investigation/

From my point of view if we require something equivalent ot SN(R) = 5-7 at G=18, that will significantly reduce the number of required repeats, while still being scientifically useful.

@deisenstein
Copy link

deisenstein commented Sep 20, 2024 via email

@segasai
Copy link
Contributor Author

segasai commented Sep 20, 2024

Looking at specifically very first exposure of the night.

image

It seems that when they are marked for repeats they are typically significantly worse than usual:

Overall given the tight sequence for the tiles requiring just one exposure and the fact that exposures on tiles requiring repeats always are below in the SNR vs mag space tells me that we maybe can really just adjust the GOALTIME for backup, and this will fix the original problem.

@schlafly
Copy link
Contributor

@rongpu , just adding you to this thread in case you want to weigh in on the DESI E(B-V) program in the context of the backup effective times.

@rongpu
Copy link
Contributor

rongpu commented Sep 26, 2024

The impact on the DESI dust mapping should be relatively small except for the very high extinction regions. If we no longer compensate the exposure time for E(B-V), at E(B-V)=0.2 we may expose for ~half as long if the EFFTIME calculation is based on g band (and the actual TSNR2 template is probably much redder and the change in exposure time smaller), and based on the Y2 data we will only lose ~10% of the backup stars (assuming the same SN_B>5 cut).

More context: most (~40%) of the backup stars are removed due to saturation in the imaging, and then ~30% are removed due to being outside the optimal stellar parameter range. Below is the SN_B histogram for all backup stars and those selected for reddening measurement. So if the priority is dust mapping, optimizing the selection would be much more important.
image

@segasai
Copy link
Contributor Author

segasai commented Sep 27, 2024

In the spirit of something concrete & discussion on slack,
I propose the following exercise, which I may be able to do myself at some point (when i have time), but happy for others to step in.
The idea is to take a subset of first exposures of backup tiles and then with the extinction fix from this PR check how the fraction of tiles marked for repeated observations would change as a function of GOALTIME.

@segasai
Copy link
Contributor Author

segasai commented Sep 30, 2024

A further followup on the discussion on slack/testing the hypothesis that we can improve the situation with extinction fix and small adjustment of GOALTIME.

Here's the EFFTIME_SPEC distribution for low extinction tiles (ebv <0.05) split into first exposures that are considered done, vs marked for repeats. This plot suggests that if we just reduced GOALTIME to reduce number of repeats, we'd need to do it drastically to significantly change the situation (which is I think undesirable).

image

However if we look at high extinction tiles (EBV>.1)
image

We clearly see that the extinction plays a very big role in requiring repeats.
So my suggestion is just to go ahead with removing extinction in teff calculation for backup, while keep the rest the same. That should noticeably improve the situation. From what I can see it should reduce the fraction of tiles requiring repeats from 80% to 50%. (CORRECTION: reduction from 80% to 50% is for high extinction tiles, but averaged across all tiles the change will be smaller, from 60% to 50%)

@schlafly
Copy link
Contributor

I'm in favor of making the easy change to the definition of EFFTIME_SPEC for the backup tiles. Can we also get the corresponding fix to the ETC in, though, so that EFFTIME_ETC and EFFTIME_SPEC remain in rough sync?

We'll also probably want to trigger a large run where we recompute the EFFTIME_SPEC for ~all of the backup tiles, to update them to use the no-extinction varieties.

@segasai
Copy link
Contributor Author

segasai commented Sep 30, 2024

@schlafly
Copy link
Contributor

Yes, we tried to get this to work, but I feel like we must have failed; I think I have found that EFFTIME_ETC / EFFTIME_SPEC doesn't depend on extinction.

@julienguy
Copy link
Contributor

So, do we have a consensus to merge this PR? (which ignore extinction in the TSNR of the backup targets).
I presume the change on the mean TSNR in low extinction region is small enough that we do not have to recalibrate the relation between TSNR and EFFTIME_SPEC for the backup program?

@segasai
Copy link
Contributor Author

segasai commented Sep 30, 2024

@schlafly On the ETC, is there a way to run it somehow to test what's going on ? desietc doesn't seem to have a test suite, so I don't quite know how one/I could test if/what ebv does.

@schlafly
Copy link
Contributor

schlafly commented Oct 1, 2024

@dkirkby , is there a way to run the ETC offline, post-facto?

@dkirkby
Copy link
Member

dkirkby commented Oct 1, 2024

Yes. If you pip-install the desietc package, it installs an etcreplay command-line script. There are some basic instructions in the comments here.

@geordie666
Copy link
Contributor

@sbailey: The surveyops team thinks we can merge this PR and the associated PR desihub/desietc#14. There will then be some additional work that needs done:

  • We’ll need to install the new ETC at KPNO (I think maybe Klaus has done this in the past?).
  • We'll need to back-calculate all the effective times for the BACKUP tiles. I'm not sure who is the correct person to ask to do that.

@sbailey
Copy link
Contributor

sbailey commented Oct 14, 2024

We’ll need to install the new ETC at KPNO (I think maybe Klaus has done this in the past?).

Since Klaus is busy with bright time testing, please coordinate with him for when that can be done by whom, then merge+deploy whenever everyone is ready and available. i.e. the ball is in your court to coordinate this to completion. Thanks.

We'll need to back-calculate all the effective times for the BACKUP tiles. I'm not sure who is the correct person to ask to do that.

Please open a new ticket defining what needs to be done (or re-post what ticket that is). IIUC this is a cleanup step for the record, but doesn't have to be closely coordinated with the merge+deploy. Is that correct? It's not particularly viable to just rerun the tsnr afterburner on everything, so if there is a way that this could be done as a surgical tile-by-tile single-quantity patch, that might be preferable.

@geordie666
Copy link
Contributor

geordie666 commented Oct 14, 2024

And, yes, the second thought is a clean-up step that does not need to be coordinated with the merge+deploy and could be considered a separate issue for later discussion.

@geordie666
Copy link
Contributor

@dkirkby: Are you comfortable with Sergey's change in desihub/desietc#14? Klaus will want your sign off before we deploy a new version of the ETC on the mountain.

@schlafly
Copy link
Contributor

IIUC this is a cleanup step for the record, but doesn't have to be closely coordinated with the merge+deploy. Is that correct? It's not particularly viable to just rerun the tsnr afterburner on everything, so if there is a way that this could be done as a surgical tile-by-tile single-quantity patch, that might be preferable.

Thinking a little about what tiles we would want to do:

  • There are 1497 observed backup tiles.
  • 1458 have more than zero effective time and will get different effective times with this PR.
  • 107 also have less than 51 s effective time and so the NTS will ask for more observations of them, and this PR will change how much more effective time gets requested. I think doing the 107 is reasonably high priority. Doing all 1458 would be better in that it would make for a consistent meaning of EFFTIME_SPEC, but it's not as important.

Is 107 surgical enough?

@sbailey
Copy link
Contributor

sbailey commented Oct 15, 2024

107 is surgical, and I'm also fine with running over all 1458 if the update is isolated to "run this code on these files and update this single column". That is especially true if someone from surveyops or backup program folks could take the lead for what needs to be patched and how. I'm trying to avoid "Anthony please rerun the tsnr afterburner on all nights that had a backup tile" just to pickup this one change.

@dkirkby
Copy link
Member

dkirkby commented Oct 16, 2024

I have just tagged 0.1.19 of the desietc package that includes desihub/desietc#14.

The next step is for Klaus to install and deploy this version on the mountain, then it should be live.

@geordie666
Copy link
Contributor

Klaus has installed and deployed the new version of the ETC on the mountain, so I'm merging this PR.

@geordie666 geordie666 merged commit 730aa44 into main Oct 16, 2024
26 checks passed
@geordie666 geordie666 deleted the backup_efftime_fixes branch October 16, 2024 16:51
@akremin
Copy link
Member

akremin commented Oct 22, 2024

Since there was no issue associated with this PR, I'll post this comment here.

I see this PR changed the EFFTIME_SPEC calculation and a related PR changed the EFFTIME_ETC calculation. Should the EFFTIME_GFA calculation also be updated?

If yes, the the relevant line(s) to modify are here:

efftime_backup = (

Which is inside the function compute_efftime that starts here:

def compute_efftime(table,

The tsnr afterbuner calls compute_efftime to calculate it here:

efftime_dark, efftime_bright, efftime_backup = compute_efftime(tsnr2_expid_table[jj])

@schlafly
Copy link
Contributor

Yes, I think it would be better to also update that.

It doesn't affect operations directly but ideally all the different EFFTIMEs should be tabulating roughly the same things.

@segasai
Copy link
Contributor Author

segasai commented Oct 22, 2024

FWIW I've created the PR #2395 that gets rid of the E(B-V) factor for backup in efftime.py

@geordie666
Copy link
Contributor

@akremin: Are we satisfied that all the EFFTIMEs are now in sync? If so can we finally close this issue?

@akremin
Copy link
Member

akremin commented Oct 30, 2024

Yes, all three EFFTIME's are now computed without the E(B-V) term. We may want to check that these three are consistent with one another once we have larger statistics (we currently only have one or two backup tiles observed since we updated the GFA efftime). But that will take some time and shouldn't prevent us from closing this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.