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

Unify main.py and survey.py #42

Closed
lpsinger opened this issue Apr 24, 2021 · 7 comments
Closed

Unify main.py and survey.py #42

lpsinger opened this issue Apr 24, 2021 · 7 comments
Assignees

Comments

@lpsinger
Copy link
Contributor

It seems like there are very few important differences between these two files. The only differences that I can see are minor variations in the command line arguments.

@mcoughlin
Copy link
Collaborator

How do you feel about a class representing the survey? Right now, you calculate the times and such in main.py, which is currently factored out into the Survey class.
https://github.com/nasa/dorado-scheduling/blob/main/dorado/scheduling/models.py
This is the main difference currently. I think once this is settled. We can get these merged.

@lpsinger
Copy link
Contributor Author

I am OK with exposure time being part of the Mission class. I want the time step to be part of the command line arguments, though. Same for the HEALPix resolution. Also I now am drawing a distinction between the HEALPix grid that is used for spatial calculations and the tessellation grid that specifies the allowed pointings of the telescope.

@mcoughlin
Copy link
Collaborator

p.add_argument('--skygrid-step', type=u.Quantity, default='0.0011 sr',
               help='Sky grid resolution (any solid angle units')
p.add_argument('--skygrid-method', default='healpix',
               choices=[key.replace('_', '-') for key in skygrid.__all__],
               help='Sky grid method')

Maybe you also want to support one of the tiling files as an optional option? This would help bring it into alignment.

@lpsinger
Copy link
Contributor Author

Why do we need to support providing the list of tiles as a file? Generating the tile grid is nearly instantaneous.

@mcoughlin
Copy link
Collaborator

But presumably we want a tile file like ZTF's (and UVEX) for the facilities that will pin the grid? Even if the grid is reproducible, people will want the file..

@lpsinger
Copy link
Contributor Author

This is a good point. We can use argparse's add_mutually_exclusive_group to select between those two alternatives. Would you like to do the honors of preparing a PR to merge these two scripts?

@mcoughlin
Copy link
Collaborator

Fixed by #44

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants