Update healpixgrp() in path.py to group by 1000 instead of by 100. #23
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
==========================================
- Coverage 49.96% 49.49% -0.47%
==========================================
Files 12 12
Lines 1283 1299 +16
Branches 263 266 +3
==========================================
+ Hits 641 643 +2
- Misses 558 572 +14
Partials 84 84
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@dnidever
This looks ok to me. I've added some small comments regarding setting a default value of None to kwargs.get in case they aren't present.
Can you also please add some tests to ensure these functions work? You can take a look at the test_special_function in tests/path/test_path.py for some examples. You can try to add new entries here. If you need help with this, let me know.
|
|
||
| ''' | ||
|
|
||
| telescope = kwargs.get('telescope') |
There was a problem hiding this comment.
To ensure that telescope is set to None if it is not present in the kwargs, try telescope = kwargs.get('telescope', None). Otherwise this will throw an uncaught error.
| if telescope is not None: | ||
| prefix = {'apo25m':'ap', 'apo1m':'ap', 'lco25m':'as'}[telescope] | ||
| return prefix | ||
| instrument = kwargs.get('instrument') |
There was a problem hiding this comment.
Similar here as with previous comment on telescope
|
|
||
| ''' | ||
|
|
||
| telescope = kwargs.get('telescope') |
There was a problem hiding this comment.
Similar here as with previous comments.
| return '' | ||
|
|
||
| def apginst(self, filetype, **kwargs): | ||
| ''' Returns APOGEE "instrument" from "telescope". |
There was a problem hiding this comment.
For this function to work, it needs a telescope kwarg. Are there any paths that do not have telescope but have instrument as a kwarg? If so, is the plan to migrate those over to use telescope and this function?
We decided to use nside=128 for the APOGEE apStar HEALPix scheme because other the HEALPix directories for the MW midplane would have had too many stars. At nside=128 we have 196608 HEALPix and it makes more sense to sort them by 1000s than 100s. So I modified healpixgrp() in path.py to divide healpix by 1000.