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

Correct output resolution matrix #132

Merged
merged 4 commits into from
May 23, 2024
Merged

Conversation

p-slash
Copy link
Contributor

@p-slash p-slash commented Jun 13, 2023

This PR redesigns the test for the new resolution matrix output by comparing each row to theoretically expected values.

@weaverba137
Copy link
Member

@julienguy, could you please review this? it is possible this fixes #137.

@weaverba137 weaverba137 force-pushed the correct-output-resolution-matrix branch from f6550b8 to f6a457c Compare May 17, 2024 18:06
@weaverba137
Copy link
Member

I have rebased this branch on main so that test will now run automatically.

@coveralls
Copy link

coveralls commented May 17, 2024

Coverage Status

coverage: 74.084%. remained the same
when pulling dc89b5c on correct-output-resolution-matrix
into 51f64f2 on main.

@weaverba137
Copy link
Member

@p-slash, apparently we decided to work on this at the exact same time. Please let me know what your next steps are.

@p-slash
Copy link
Contributor Author

p-slash commented May 23, 2024

Thanks @weaverba137 . This fix was needed after PR #128 where I changed how the input resolution matrix was downsampled to DESI resolution. It compares the downsampled resolution matrix to theoretical expectations, though a relative tolerance of 0.1% is needed to pass this test.

As far as I know, these two PRs affect only Lya forest P1D, and we have been happy about it.

@weaverba137
Copy link
Member

@p-slash, OK, if you have no objections, I'll clean up and merge this PR.

@weaverba137
Copy link
Member

I'm also adding a quick fix for #133.

@p-slash
Copy link
Contributor Author

p-slash commented May 23, 2024

No objections. Thanks

@weaverba137 weaverba137 merged commit 0540ad2 into main May 23, 2024
54 checks passed
@weaverba137 weaverba137 deleted the correct-output-resolution-matrix branch May 23, 2024 20:43
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.

3 participants