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.

Sign up for GitHub

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

Jump to bottom

DM-15008: anetAstrometry.py uses self.distortionContext, which does not exist #11

Merged
merged 8 commits into from
Jul 11, 2018
Merged

DM-15008: anetAstrometry.py uses self.distortionContext, which does not exist #11

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7ba677b
git hide .coverage
r-owen Jul 3, 2018
44497a4
Remove obsolete reference to distortionContext
r-owen Jul 3, 2018
8def5fb
Remove obsolete reference to wcs.hasDistortion()
r-owen Jul 3, 2018
4b87720
Use updateSourceCoords
r-owen Jul 6, 2018
7c0a5ed
Add cleanBadPoints.py to this package
r-owen Jul 6, 2018
77a9b18
Fix sigma-0 bug in `clean`
r-owen Jul 6, 2018
67b4993
Add a basic unit test for ANetAstrometryTask
r-owen Jul 6, 2018
7f16872
Pass log args separately in ANetAstrometryTask
r-owen Jul 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions python/lsst/meas/extensions/astrometryNet/anetAstrometry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,9 +435,8 @@ def fitWcs(initialWcs, title=None):
except lsst.pex.exceptions.LengthError as e:
self.log.warn("Unable to fit SIP: %s" % e)

self.log.info("Astrometric scatter: %f arcsec (%s non-linear terms, %d matches, %d rejected)" %
(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))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By no means is this necessary, but it would be nice to see either a format method used here or a python f string

Copy link
Member

@TallJimbo TallJimbo Jul 11, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it does. Use log.infof().

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@r-owen r-owen Jul 11, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@r-owen r-owen Jul 11, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

exposure.setWcs(wcs)

# Apply WCS to sources
Expand Down