DM-15008: anetAstrometry.py uses self.distortionContext, which does not exist #11
Conversation
Move it from meas_astrom since this is the only package using it.
| (scatter.asArcseconds(), "with" if wcs.hasDistortion() else "without", | ||
| len(matches), numRejected)) | ||
| self.log.info("Astrometric scatter: %f arcsec (%d matches, %d rejected)" % | ||
| (scatter.asArcseconds(), len(matches), numRejected)) |
There was a problem hiding this comment.
By no means is this necessary, but it would be nice to see either a format method used here or a python f string
There was a problem hiding this comment.
Note that this would also work with , instead of % and no tuple around the formatting arguments (i.e. just make them extra arguments to log.info). Our loggers can do the formatting themselves.
That's how it's usually done in Python, but I'm not sure it's actually better (it is in C++, where the ability to skip the formatting when that logger is disabled is important).
There was a problem hiding this comment.
Unfortunately {} formatting doesn't work with lsst.log when the arguments are provided separately, and in my opinion that's a useful enough feature to justify using the older style formatting. I have updated the existing log messages to pass the arguments separately, in case it speeds things up if the message is not printed.
There was a problem hiding this comment.
Im confused, how does that not work, as format is a method on the string itself, so log.info should only ever see that a string was passed into the function call. The final string should have already been realized at that point.
There was a problem hiding this comment.
The point is that I want to pass the arguments separately and let log apply them if and only if the message is to be logged. log.info handles % formatting that way. Apparently log.infof handles {} formatting but I've already updated the code to pass the arguments separately to log.info and the like, and plan to leave it at that. This package is under consideration for being retired anyway, and I don't want to spend more time on it (especially making changes that might break something).
There was a problem hiding this comment.
@r-owen, I think the confusion here (for me at least) is just coming from the fact that you now have an actual % between your string and the arguments you want to insert into it, so you aren't actually letting the logger apply them conditionally. Continuing with %-style logging via the logger still seems fine to me; we certainly haven't made any statements about trying to phase that out, despite the new options available.
There was a problem hiding this comment.
I agree with @TallJimbo that the idiom for log.info is to use %-formatting syntax but not do the formatting. Remove the % and the tuple and have the extra items to be formatted as normal arguments.
There was a problem hiding this comment.
I'll try to be clearer. I agreed with @TallJimbo that it's better to let the logger do the formatting, so I pushed a commit that changes all logging messages in this task to do that. I chose to retain the % formatting codes in the message strings, rather than change to {...} in the message strings and calling the f suffix methods such as log.infof. Please look at the new code instead of the snippet above, which is outdated.
| # all remaining points are good; short circuit | ||
| if newidx: | ||
| return newidx | ||
| return idx |
There was a problem hiding this comment.
This might look cleaner as:
return newidx if newidx else idx
it would fail instead of accepting all points
so the formatting can be deferred until it is known if the message is to be logged.
No description provided.