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-39604: Record full noise matrix in PTC dataset #199

Merged
merged 7 commits into from Jun 15, 2023
Merged

Conversation

plazas
Copy link
Contributor

@plazas plazas commented Jun 13, 2023

No description provided.

@plazas plazas requested a review from erykoff June 13, 2023 16:34
Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

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

Please add a check of the noise matrix in the test_ptc.py code. It can be empirical, but do check that it was computed and the numbers are sane and consistent.

@plazas plazas requested a review from erykoff June 14, 2023 20:13
@@ -253,6 +258,30 @@ def test_covAstier(self):
extractPtc.covariancesSqrtWeights[ampName][0],
ptc.covariancesSqrtWeights[ampName][i],
)
Copy link
Contributor

Choose a reason for hiding this comment

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

assertFloatsAlmostEqual can work with arrays, this can be just one call for each; or maybe you're only checking the diagonals? Wait, this is a dict ... that isn't correct.

noiseMatrixExpected = np.array(
    [[29.37, 0, 0],
     [0, -23.209, 0],
     [0, 0, 35.69]]
)

above (or maybe do the full matrix with off-diagonals?) would certainly be a cleaner comparison (and also for the noB case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I now use the full matrix too.

@erykoff
Copy link
Contributor

erykoff commented Jun 14, 2023

I assume you ran black on that file to clean up the formatting? If so, that must be a separate commit, or else it's very hard to follow what is substance and what is style in the git history. Thanks!

@plazas
Copy link
Contributor Author

plazas commented Jun 15, 2023

@erykoff Yes, I had run Black along with the changes. I have reverted the commit, and now there are two separate commits. Please let me know if it looks good now and if I can merge after running Jenkins!

@plazas plazas merged commit 58e228e into main Jun 15, 2023
1 check passed
@plazas plazas deleted the tickets/DM-39604 branch June 15, 2023 18:49
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