Skip to content
This repository has been archived by the owner on Oct 21, 2024. It is now read-only.

Add unit test for sky grid module #56

Merged
merged 2 commits into from
Apr 30, 2021
Merged

Conversation

lpsinger
Copy link
Contributor

Also, fix conversion of area to linear scale in sinusoidal grid method. The new test_skygrid unit test fails without this.

Where was the factor of 2/pi from, @mcoughlin? Is this a bug in sinusoidal, or a bug in the new test?

@lpsinger lpsinger requested a review from mcoughlin April 30, 2021 01:11
@codecov
Copy link

codecov bot commented Apr 30, 2021

Codecov Report

Merging #56 (1a486a7) into main (529dd1e) will increase coverage by 4.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   27.05%   31.18%   +4.13%     
==========================================
  Files          27       28       +1     
  Lines        1327     1337      +10     
==========================================
+ Hits          359      417      +58     
+ Misses        968      920      -48     
Impacted Files Coverage Δ
dorado/scheduling/skygrid/_sinusoidal.py 100.00% <100.00%> (+66.66%) ⬆️
dorado/scheduling/tests/test_skygrid.py 100.00% <100.00%> (ø)
dorado/scheduling/skygrid/_healpix.py 100.00% <0.00%> (+40.00%) ⬆️
dorado/scheduling/skygrid/_spiral.py 100.00% <0.00%> (+44.44%) ⬆️
dorado/scheduling/skygrid/_geodesic.py 97.56% <0.00%> (+78.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 529dd1e...1a486a7. Read the comment docs.

Copy link
Collaborator

@mcoughlin mcoughlin left a comment

Choose a reason for hiding this comment

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

I presume some conversion when there was a mix of different kinds of FOV. Thanks for catching this!

@lpsinger
Copy link
Contributor Author

Yeah, that makes sense. area = pi r^2, r = sqrt(area) / pi, diameter = 2 / pi * sqrt(area).

@lpsinger lpsinger merged commit b94be67 into nasa:main Apr 30, 2021
@lpsinger lpsinger deleted the test-skygrid branch April 30, 2021 01:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants