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

Add tiling example #5

Merged
merged 7 commits into from
Mar 16, 2021
Merged

Add tiling example #5

merged 7 commits into from
Mar 16, 2021

Conversation

mcoughlin
Copy link
Collaborator

Quick example to make a tiling on the sphere

Copy link
Contributor

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

Nice, but we'll have to move this to a module eventually.

@@ -137,7 +137,7 @@ To install with [Pip]:
5. To generate an animated visualization for this observing plan, run the
following command:

$ dorado-scheduling-animate exaples/6.fits examples/6.ecsv examples/6.gif
$ dorado-scheduling-animate examples/6.fits examples/6.ecsv examples/6.gif
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! I just fixed that on master, so please rebase.

return p

def tesselation_spiral(FOV_size, scale=0.80):
FOV = FOV_size*FOV_size*scale
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix these lint errors.

dorado/scheduling/scripts/tile.py Outdated Show resolved Hide resolved
Comment on lines 42 to 48
points = np.zeros((n, 3))
points[:,0] = radius * np.cos(theta)
points[:,1] = radius * np.sin(theta)
points[:,2] = z

ra, dec = hp.pixelfunc.vec2ang(points, lonlat=True)
return ra, dec
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
points = np.zeros((n, 3))
points[:,0] = radius * np.cos(theta)
points[:,1] = radius * np.sin(theta)
points[:,2] = z
ra, dec = hp.pixelfunc.vec2ang(points, lonlat=True)
return ra, dec
coords = SkyCoord(radius, theta * u.rad, z, representation_type='cylindrical')
coords.representation_type = 'unitspherical'
return coords

dorado/scheduling/scripts/tile.py Show resolved Hide resolved
dorado/scheduling/scripts/tile.py Outdated Show resolved Hide resolved
@lpsinger
Copy link
Contributor

Would you mind plotting the output of this please, to check?

mcoughlin and others added 2 commits March 11, 2021 15:04
Co-authored-by: Leo Singer <leo.singer@ligo.org>
Co-authored-by: Leo Singer <leo.singer@ligo.org>
@codecov-io
Copy link

codecov-io commented Mar 11, 2021

Codecov Report

Merging #5 (da3cee5) into master (44a33ed) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #5   +/-   ##
=======================================
  Coverage   89.70%   89.70%           
=======================================
  Files           6        6           
  Lines         136      136           
=======================================
  Hits          122      122           
  Misses         14       14           

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 44a33ed...da3cee5. Read the comment docs.

@lpsinger
Copy link
Contributor

Here's what this looks like near the north celestial pole. It has a lot of overlap. Does this look correct?

image_2021_03_12T01_05_54_932Z

@mcoughlin
Copy link
Collaborator Author

Yeah I just added one that I think behaves a bit better at the poles. Can we add that script to tiles.py for the plotting?

@lpsinger
Copy link
Contributor

What is the provenance of these algorithms? Have they been used for previous surveys?

@mcoughlin
Copy link
Collaborator Author

I have used a variation on these to produce tilings for various telescopes, including DECam and others. I usually tune the overlaps by hand a bit for the various surveys to get some reasonable amount of overlap, which also means needing to play around a bit close to the poles.

@lpsinger
Copy link
Contributor

Would you please add docstrings with references to the journal articles where these grids are introduced, described, or used?

return coords


def tesselation_packing(FOV_size, scale=0.97):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty generic name.

@lpsinger
Copy link
Contributor

I'm ready to merge this as soon as legal processes your CLA. So sorry about the red tape.

@lpsinger
Copy link
Contributor

@mcoughlin
Copy link
Collaborator Author

awesome leo!

@lpsinger
Copy link
Contributor

They finally got your CLA processed. I'd like to see this unified with the new dorado.scheduling.tesselation module, but since I added that after you had opened this PR, I could merge this as is and do the refactoring myself. What would you prefer?

@mcoughlin
Copy link
Collaborator Author

Let's get this merged and then I can help as necessary this weekend when I have more time. Thanks Leo.

@lpsinger lpsinger merged commit c750182 into nasa:master Mar 16, 2021
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

3 participants