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

Refactor and clean up GROWTH tiling schemes #9

Merged
merged 1 commit into from
Mar 23, 2021

Conversation

lpsinger
Copy link
Contributor

@lpsinger lpsinger commented Mar 23, 2021

Refactor and clean up GROWTH tiling schemes

  • Move functions to the dorado.scheduling.tesselation module.
  • Clean up existing GROWTH methods.
  • Update all methods to take a single resolution parameter, area.
  • Convert docstrings to Numpydoc format.
  • Add example code.
  • Add to Sphinx project documentation.
  • Use Astropy quantities for angle arguments.
  • Write output in ECSV format.
  • Remove logger.

@lpsinger lpsinger marked this pull request as draft March 23, 2021 14:36
@lpsinger
Copy link
Contributor Author

@mcoughlin, I would like to unify the options for all three tiling methods (the geodesic tiling and the two spiral tilings). The geodesic tiling has one natural argument: the desired number of tiles. The reason that this is the natural argument is that it allows only certain integer values, and it must solve for the actual number of tiles given the requested minimum number of tiles.

The other two tilings take two arguments (fov, scale), but both of those arguments really control the average areal density of the tiles.

What would be the most natural argument(s) for all three tilings to take?

@mcoughlin
Copy link
Collaborator

@lpsinger The "scale" term is redundant and a holdover from using the FOV in the config files. I do think something equivalent to "spacing between tiles" is what we are after in the end.

@lpsinger
Copy link
Contributor Author

OK, then how about number of tiles, so that it's dimensionless?

@mcoughlin
Copy link
Collaborator

My only concern is that people may not intuitively know how to calculate the number of tiles for a given FOV. Do we provide them some guidance maybe?

@lpsinger
Copy link
Contributor Author

So maybe area per tile is better?

@mcoughlin
Copy link
Collaborator

yes that sounds good to me.

@lpsinger
Copy link
Contributor Author

The phi_theta_spiral method is misnamed, isn't it? It's just a regular grid on a Lambert projection, that is to say, a regular grid in right ascension and cosine declination?

@lpsinger
Copy link
Contributor Author

I misspoke. The phi_theta_spiral is just a uniform grid on a sinusoidal projection, right, @mcoughlin?

@mcoughlin
Copy link
Collaborator

Yes this is more accurate.

- Move functions to the dorado.scheduling.tesselation module.
- Clean up existing GROWTH methods.
- Update all methods to take a single resolution parameter, area.
- Convert docstrings to Numpydoc format.
- Add example code.
- Add to Sphinx project documentation.
- Use Astropy quantities for angle arguments.
- Write output in ECSV format.
- Remove logger.
@lpsinger lpsinger changed the title DRAFT: Refactor and clean up GROWTH tiling schemes Refactor and clean up GROWTH tiling schemes Mar 23, 2021
@lpsinger lpsinger marked this pull request as ready for review March 23, 2021 19:26
@lpsinger
Copy link
Contributor Author

@mcoughlin, I've updated all of the methods to take an area parameter.

Please review both the golden angle spiral and the sinusoidal grid methods. I have reimplemented them to make the code more transparent.

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.

Really love this Leo. Will be great for the users.

@lpsinger
Copy link
Contributor Author

Great! FYI I made some small changes to the indexing in the sinusoidal routine, to improve handling of the boundaries in ra and dec.

@lpsinger lpsinger merged commit d52019a into nasa:master Mar 23, 2021
@lpsinger lpsinger deleted the tesselation-cleanup branch March 23, 2021 20:00
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.

2 participants