sbailey / surveyqa

DESI Survey QA Dashboard
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Added hourangle and impovements to summary #37

Closed williyamshoe closed 5 years ago

williyamshoe commented 5 years ago

core.py: -added HOURANGLE column to the exposures table

summary.py: -added strict bounds on the vertical cursor line on the progress plots -added histograms for SKY, HOURANGLE, and total expose time for each tile

sbailey commented 5 years ago

When I run this it crashes with

WARNING: Cannot aggregate column 'PROGRAM' with type '|S6' [astropy.table.groups]
Traceback (most recent call last):
  File "/Users/sbailey/desi/git/surveyqa/bin/surveyqa", line 35, in <module>
    surveyqa.core.makeplots(exposures, tiles, args.outdir)
  File "/Users/sbailey/desi/git/surveyqa/py/surveyqa/core.py", line 40, in makeplots
    surveyqa.summary.makeplots(exposures, tiles, outdir)
  File "/Users/sbailey/desi/git/surveyqa/py/surveyqa/summary.py", line 745, in makeplots
    expTimePerTile_plot = get_expTimePerTile(exposures, 500, 250)
  File "/Users/sbailey/desi/git/surveyqa/py/surveyqa/summary.py", line 580, in get_expTimePerTile
    total_exptime_dgb("DARK", "red")
  File "/Users/sbailey/desi/git/surveyqa/py/surveyqa/summary.py", line 576, in total_exptime_dgb
    a = a[a["PROGRAM"] == string]
  File "/Users/sbailey/anaconda3/envs/desi/lib/python3.6/site-packages/astropy/table/table.py", line 1222, in __getitem__
    return self.columns[item]
  File "/Users/sbailey/anaconda3/envs/desi/lib/python3.6/site-packages/astropy/table/table.py", line 106, in __getitem__
    return OrderedDict.__getitem__(self, item)
KeyError: 'PROGRAM'

I have not previously used Table.group_by().group.aggregate so I can't help you much, but it appears that is where it is messing up your table. Perhaps you need to filter exposures_nocalib by PROGRAM first, and then do the 'group_by` while including only the TILEID and EXPID columns but not the PROGRAM column?

Even though total_exptime_dgb is an internal function, it would be useful to have a brief docstring and/or comments and/or a more informative input variable name than "string").

williyamshoe commented 5 years ago

My program was not providing the error you listed above at first, but then I tried updating astropy and now the error shows. It should now work fine (the error was that in the new version, they used str for their string type columns, but in the previous version, they used np.str__ for their string type columns).

I have also added docstrings to the internal functions, and gave a more appropriate variable name to string.

sbailey commented 5 years ago

Thanks for these updates and the astropy workaround for str vs. np.str__. I'll merge this now, but in the future please don't mix algorithmic updates with layout updates. In particular moving the locations of the plots might cause a merge conflict with the layout updates we discussed with @ana-lyons last week. Ideally this PR would have been 4 PRs:

That's a lot more steps than one would do if working on a project by yourself, but keeping the topics isolated and code changes-per-PR minimal, it helps coordinate with others and avoid potential merge conflicts.