search

desihub / specsim

Quick simulations of spectrograph response
2 stars 9 forks source link

Correct output resolution matrix #132

Closed p-slash closed 6 months ago

p-slash commented 1 year ago

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

weaverba137 commented 6 months ago

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

weaverba137 commented 6 months ago

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

coveralls commented 6 months ago

Coverage Status

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

weaverba137 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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

weaverba137 commented 6 months ago

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

p-slash commented 6 months ago

No objections. Thanks