Conversation
geordie666
left a comment
geordie666
left a comment
There was a problem hiding this comment.
Thanks for your time-critical work on this @jbanovetz. And, thanks for addressing my previous (offline) review.
I have a few more comments for you to adopt (or consider) before merging this PR.
py/desitarget/ToO.py
Outdated
|
|
||
| #JB enable subtables past a date (in this case corresponding to 2026/01/28) | ||
| todaymjd = Time.now().mjd | ||
| if todaymjd >= Time('2026-01-28').mjd: |
There was a problem hiding this comment.
To ensure this works automatically for reproducibility (e.g. for the alt-MTLs, etc.) you want to set this to a date in the future, as we already ran the ToOs for yesterday and today. I'd suggest '2026-01-30' and we'll try to squeeze in the first new update tomorrow?
There was a problem hiding this comment.
We should probably make this '2026-01-31', now. I can monitor the process on Saturday to check nothing fails.
py/desitarget/ToO.py
Outdated
|
|
||
|
|
||
| def ledger_to_targets(toodir=None, survey="main", ecsv=True, outdir=None): | ||
| def ledger_to_targets(toodir=None, survey="main", ecsv=True, outdir=None, subtable=False): |
There was a problem hiding this comment.
As you're hardcoding subtable to always trigger after a specific date, it doesn't make sense to also include it as a kwarg. I'd remove the kwarg.
py/desitarget/ToO.py
Outdated
| outdir : :class:`str`, optional, defaults to ``None`` | ||
| If passed and not ``None``, then read the input ledger from | ||
| `toodir` but write the file of targets to `outdir`. | ||
| subtable : :class:`bool`, optional, defaults to ``True`` |
There was a problem hiding this comment.
Once you've deleted the subtable kwarg, remember to also remove subtable from the docstring.
py/desitarget/ToO.py
Outdated
| # ADM write out the results. | ||
| _write_too_files(fn, outdata, ecsv=ecsv) | ||
| # JB added if statement for writing out a subtable | ||
| if subtable: |
There was a problem hiding this comment.
I'm OK with your approach if you find it easiest to parse, but consider whether you need two if statements; one for setting subtable and one for using it. Particularly as the subtable kwarg seems unnecessary.
Every relevant line of code for subsampling the data could presumably be embedded in a single if todaymjd >= Time('2026-01-28').mjd: block where you currently have the if subtable: block.
|
Thank you for reviewing! I agree with all the changes and have implemented them. Let me know if there is anything else that needs to be put in! |
|
Thanks @jbanovetz. This looks good I'm going to merge and tag now. |
3 participants
This update will divide the ToO-input.ecsv list into a primary folder that grows over time (anything with
-allin the name) and a smaller folder of just targets that are available right now. Fiber assign on the fly will use the smaller folders for assigning ToOs, allowing for more ToOs to be uploaded in a day.