DM-35619: Add astrometry residuals with refCat plots #15
Conversation
cmsaunders
force-pushed
the
tickets/DM-35619
branch
from
fc32a7b
to
f30fc40
Compare
cmsaunders
force-pushed
the
tickets/DM-35619
branch
6 times, most recently
from
006d27a
to
1fdabb8
Compare
| dec=refCatalog['coord_dec'] * u.degree) | ||
|
|
||
| sourceCat_ap = SkyCoord(ra=targetCatalog['coord_ra'] * u.degree, | ||
| dec=targetCatalog['coord_dec'] * u.degree) |
There was a problem hiding this comment.
Should we have spaces around the * here?
There was a problem hiding this comment.
Do we know that the units are always gong to be degrees or should it be a config option?
There was a problem hiding this comment.
My mind is blown -- somehow I have been doing spacing around operators wrong.
I added a config option for the units.
There was a problem hiding this comment.
When you switch the code to using black you will find that that rule about spaces around operators goes away anyhow... (and this package is using black)
| ) | ||
|
|
||
| matchCatalog = pipeBase.connectionTypes.Output( | ||
| doc="Catalog with matched target and reference objects with separations", |
There was a problem hiding this comment.
I think I would prefer matchedCatalog as it reinforces that this is the one that has been matched rather than the one to match but I don't think it really matters.
| for selector in selectorAction: | ||
| for band in ["g", "r", "i", "z", "y"]: | ||
| selectorSchema = selector.getFormattedInputSchema(band=band) | ||
| columns += [s[0] for s in selectorSchema] |
There was a problem hiding this comment.
Mixture of ' and ". I don't think it matters but it looks neater when it is consistent.
There was a problem hiding this comment.
This was moved to a config option (with consistent quotation marks).
| refCat = skyCircle.refCat | ||
|
|
||
| # Convert the coordinates to RA/Dec and convert the catalog to a | ||
| # dataframe |
There was a problem hiding this comment.
Are you using black, this is a thing that has a right answer, and black does it.
There was a problem hiding this comment.
Yeah, when I tried to commit, triggering pre-commit, black complained about all the places I just removed whitespaces. However, this seems to disagree with https://developer.lsst.io/python/style.html#binary-operators-should-be-surrounded-by-a-single-space-except-for. Maybe I don't have black configured correctly.
There was a problem hiding this comment.
When black is enabled for a project the Python style guide for formatting is no longer relevant. I thought there was a comment about that somewhere in the dev guide but maybe that was only in the section about using black.
There was a problem hiding this comment.
| median = np.nanmedian(value[-x:]) | ||
| if self.inputUnit != self.outputUnit: | ||
| median = (median * u.Unit(self.inputUnit)).to(u.Unit(self.outputUnit)).value | ||
| return median |
| ---------- | ||
| df : `pandas.core.frame.DataFrame` | ||
| The catalog to calculate the position difference from. | ||
| Returns |
There was a problem hiding this comment.
Variable type and name missing.
|
I've left a few minor comments, I know that you have tested this all thoroughly and that it works. One general comment is that I think the style guideline says that we shouldn't have spaces around * and / but there are spaces around them here. Perhaps black did this formatting? |
cmsaunders
force-pushed
the
tickets/DM-35619
branch
from
d357a6f
to
3c0b28c
Compare
|
@timj , do you mean in interfaces.py? |
|
@cmsaunders I was commenting on a line in a commit but that clearly became detached from the pull request so sorry about that. It was the |
|
I tried |
|
@timj in this cases @cmsaunders is using something that ducks like a bool on the class interface, but MYPY is unhappy because of LSP even though they duck the same. We could loosen up the type on the base class, but in reality we likely will move toward something similar to what @cmsaunders has on the base class, we just dont want to disrupt right before the weekly, so as we know they duck the same, for now we are just trying to inform MYPY it ok to violate LSP. |
|
@cmsaunders In light of needing yet another cast or typing or something in order to make mypy happy again, let's back up and actually do the type union on the base class. you can change https://github.com/lsst/analysis_tools/blob/main/python/lsst/analysis/tools/interfaces.py#L287 this to |
|
and remove the type ignore where we added it earlier |
cmsaunders
force-pushed
the
tickets/DM-35619
branch
from
16f454d
to
f044912
Compare
|
@natelust, I pushed a change before I saw your last message, which worked, but I'll try your suggestion and see how that works. |
cmsaunders
force-pushed
the
tickets/DM-35619
branch
from
f140ea2
to
e4def86
Compare
No description provided.