try fibermap header if (TARGT)RA isn't in primary header #934
Conversation
py/desispec/scripts/extract.py
Outdated
| @@ -170,7 +170,12 @@ def main(args): | |||
| dw = 0.7 | |||
|
|
|||
| if args.barycentric_correction : | |||
| barycentric_correction_factor = barycentric_correction_multiplicative_factor(img.meta) | |||
| try: | |||
There was a problem hiding this comment.
Personally I'd prefer an "if" statement here. If the key is in img.meta use that, else use fibermap.meta. Better yet, check both with if-elif-else where we return a more informative error in the else case where neither has it.
Try's are great for robustness but since we know the failure point here I think it's more efficient/readable to address it directly.
In thinking more this was probably done because we'd want to check for both RA and DEC keywords as well as the TARGETXX varieties, and the function could possibly fail because of other keywords (which this try statement would catch).
Even so, I think I lean toward an "if" statement.
There was a problem hiding this comment.
I originally used try/except because it was fewer lines of code, but agree that if/else is clearer code, and that is valuable. I switched it.
py/desispec/scripts/extract.py
Outdated
| @@ -334,7 +339,12 @@ def main_mpi(args, comm=None, timing=None): | |||
| dw = 0.7 | |||
|
|
|||
| if args.barycentric_correction : | |||
| barycentric_correction_factor = barycentric_correction_multiplicative_factor(img.meta) | |||
| try: | |||
There was a problem hiding this comment.
The same comment as above. Assuming these are the only two places this is done I think it's fine that they're copy-pasted.
There was a problem hiding this comment.
There is a ton of replicated code in extract.main vs. extract.main_mpi which is indeed a maintenance risk; issue #827 is about resolving that overall but in the meantime we continue to accrete more copied code. Related: I'm working with @lastephey and @ziyaointl on an extraction refactor to make it easier to port to GPUs, and that's where we'll reconcile this MPI vs. non-MPI code duplication.
This PR fixes the crash in PR #896 by trying the fibermap header if it can't find RA or TARGTRA in the primary HDU (as was the case in early commissioning data) for the purposes of barycentric corrections.
Tested on the command originally reported in #896 (20200122/00043028), and also on 20200315/00055611 (new data)