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.

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

DM-20566: Replace afwGeom with geom #128

Merged
merged 6 commits into from Jul 18, 2019
Merged

Conversation

timj
Copy link
Member

@timj timj commented Jul 18, 2019

No description provided.

@timj timj force-pushed the tickets/DM-20566-ip_diffim branch from cbc1571 to 404e25f Compare July 18, 2019 03:03
@timj timj force-pushed the tickets/DM-20566-ip_diffim branch from 404e25f to c34c92c Compare July 18, 2019 17:37
@timj timj requested a review from isullivan July 18, 2019 20:28
Copy link
Contributor

@isullivan isullivan left a comment

Choose a reason for hiding this comment

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

Looks good. There is one missing docstring conversion, and switching from lsst.afw.geom to lsst.geom messed up the alphabetization of the imports and includes in most files. I stopped commenting on those after a while, but you might want to clean those up.

#include "lsst/afw/geom.h"
#include <cmath>

#include "lsst/geom.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor point, but shouldn't this include go after the afw ones?

@@ -13,14 +13,15 @@
#define LSST_IP_DIFFIM_BUILDSPATIALKERNELVISITOR_H

#include "Eigen/Core"
#include "lsst/geom.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to after the afw includes?

@@ -16,15 +16,15 @@
#include "Eigen/Core"

#include "lsst/afw/math.h"
#include "lsst/afw/geom.h"
#include "lsst/geom.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to after the afw include?

@@ -23,7 +23,7 @@
import numpy as np
from scipy import ndimage
from lsst.afw.coord.refraction import differentialRefraction
import lsst.afw.geom as afwGeom
import lsst.geom as geom
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to after the afw import?

@@ -31,6 +31,7 @@
from . import diffimLib

# all the other LSST packages
import lsst.geom as geom
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to after the afw imports?

@@ -23,7 +23,7 @@
import numpy as np
import warnings

import lsst.afw.geom as afwGeom
import lsst.geom as geom
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to after the afw import?

@@ -21,7 +21,7 @@

import numpy as np

import lsst.afw.geom as afwGeom
import lsst.geom as geom
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to after the afw import?

@@ -787,7 +787,7 @@ def _plotBoxGrid(self, boxes, bbox, **kwargs):
Parameters
----------
boxes : `list`
a list of `afwGeom.BoundingBox`es
a list of `lsst.afw.geom.BoundingBox`es
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be lsst.geom.BoundingBox ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was meant to change to to lsst.geom.Box2I.

@timj timj merged commit ba33251 into master Jul 18, 2019
@timj timj deleted the tickets/DM-20566-ip_diffim branch July 18, 2019 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants