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-13055 Add centroid CovarianceMatrix to TestDataset #105

Merged
merged 1 commit into from Dec 21, 2017

Conversation

parejkoj
Copy link
Contributor

No description provided.

Copy link
Member

@TallJimbo TallJimbo 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! One minor query that doesn't require a change.

@@ -345,6 +348,9 @@ def addSource(self, flux, centroid, shape=None):
record = self.catalog.addNew()
record.set(self.keys["flux"], flux)
record.set(self.keys["centroid"], centroid)
offdiag = centroid[0]*centroid[1]*0.001
covariance = np.matrix(((centroid[0]*0.01, offdiag), (offdiag, centroid[1]*0.01)))
record.set(self.keys["centroid_sigma"], covariance.astype(np.float32))
Copy link
Member

Choose a reason for hiding this comment

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

Setting the centroid uncertainty to be proportional to the centroid is a little weird, as it gives the uncertainty a trend with position. Of course, the test data is highly artificial, and if you want such a trend to test something, I think that's fine - just wanted to see what you were thinking here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No deep thought process, I just needed to put something in for the errors. If you have something else you'd prefer, please let me know.

Copy link
Member

Choose a reason for hiding this comment

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

If there's already an RNG near this code, maybe just make them random? If not, don't worry about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at the change I just pushed and tell me what you think.

@parejkoj parejkoj merged commit 45b8333 into master Dec 21, 2017
@ktlim ktlim deleted the tickets/DM-13055 branch August 25, 2018 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants