Open
Conversation
cailmdaley
reviewed
Feb 5, 2026
Contributor
cailmdaley
left a comment
There was a problem hiding this comment.
Nice work modernising the CANFAR submission path and replacing CDSClient with astroquery. The documentation rewrite is a real improvement. A few things to fix before merge — mostly runtime bugs that will crash in production, plus some smaller items.
Bugs that will crash
canfar_monitor.py:130—RunTimeError→RuntimeErrorcanfar_monitor.py:188—self._get_kind()→self.get_kind()(no underscore)canfar_monitor.py:121—self._params["verbose"]referenced but never defined inparams_defaultcanfar_monitor.py:206-208—elseaftertry/exceptruns on success, not failure. The "Failed to destroy" message fires when destruction succeeds.canfar_monitor.py:222,235—"failed{estr}"missingfprefixsummary.py:218,235—mgsinstead ofmsg—NameErrorat runtimeclear_ngmix_prev.py:117— globsngmix_out_dirwhen it should globprev_out_dirdistribute_tiles.py:124— compares string args with int literals(0, 1)— will never match, should be('0', '1')
Intentional?
make_cat.py:348— threshold changed from0.9to0.0. Since a ratio can't be negative, this disables the check entirely. If intentional, the dead code block could be removed or at least commented with why.
Style / design
distribute_tiles.py:1— hardcoded shebang#!/arc/home/kilbinger/...uncompress_fits.py:62— bareexcept:— worth catching something specificuncompress_fits.py:75-76— theTFORMfix runs outside theif name:guard, so on empty keysvalue/commentare stale from the previous iterationcanfar_submit.py:32-33— the!= 512guard can never trigger (set to 512 on the line above)canfar_submit.py:260—AsyncSession()opened twice (once inrun_async, again in_submit_single_batch)pyproject.toml—requires-pythondowngraded from>=3.11to>=3.10without mention. Alsocanfar,sf_tools,h5py,pandasadded as core deps — could these live under an optional[canfar]extra?- Star imports in
summary_run.pyandsummary_params_pre_v2.py
Typos
canfar_monitor.py:3— "Montiot" → "Monitor"canfar_monitor.py:247— "Retreiving" → "Retrieving"mask.py:498— "astroquer" → "astroquery"canfar_submit.py:116— "SESSSION" (triple S)mask.py:496-501—IndexErrorraised with positional args instead of a formatted string
Tests
None of the new code has tests. The mgs/msg typo in summary.py would have been caught immediately.
Contributor
Author
|
Check again please @cailmdaley, I answered to all points of your PR report. Thanks! |
- canfar_monitor: "Montior" typo, try/except/else flow for bulk destroy - canfar_submit: "debut_out" dict key typo - uncompress_fits: header assignment inside if-name guard - distribute_tiles: check arg value not flag name for dry_run Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Hey Martin, thanks for the fixes! I took the liberty of pushing a few more small ones I spotted while verifying:
Let me know if I made any mistakes. Have a nice weekend! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR updates job hanling on canfar to the new canfar python library system.
This comes with job submission and monitoring classes and scripts.
New tiles (P9) were added to the CFIS tile list.
Closes some old issues #669 .
Reviewer Checklist
developbranch