First commit of Density test #140
Conversation
There was a problem hiding this comment.
@fjaviersanchez thanks for the PR. I made some minor suggestions/comments.
descqa/DensityVersusExtinction.py
Outdated
|
|
||
| catalog_data = catalog_instance.get_quantities(['ra', 'dec', 'extendedness']) | ||
| galaxies = catalog_data['extendedness']==1 | ||
| data_map = create_hp_map(catalog_data['ra'][galaxies], catalog_data['dec'][galaxies], self.nside) |
There was a problem hiding this comment.
Lines 34-36: (1) add has_quantities and skip test if not all needed quantities exist, (2) use filters instead of masking the data afterwards.
if not catalog_instance.has_quantities(['ra', 'dec', 'extendedness']):
return TestResult(skipped=True, summary='catalog does not have needed quantities')
catalog_data = catalog_instance.get_quantities(['ra', 'dec', 'extendedness'], filters=['extendedness == 1'])
data_map = create_hp_map(catalog_data['ra'], catalog_data['dec'], self.nside)
descqa/DensityVersusExtinction.py
Outdated
| mask = data_map>0 # This is a good approximation if the pixels are big enough | ||
| xmin = np.percentile(self.validation_data[mask], 5) | ||
| xmax = np.percentile(self.validation_data[mask], 95) | ||
| data_map = data_map/(3600*hp.nside2pixarea(self.nside, degrees=True)) # To get the density in arcmin^-2 |
There was a problem hiding this comment.
suggestion for Line 40
data_map /= (3600*hp.nside2pixarea(self.nside, degrees=True))
descqa/DensityVersusExtinction.py
Outdated
| data_map = create_hp_map(catalog_data['ra'][galaxies], catalog_data['dec'][galaxies], self.nside) | ||
| mask = data_map>0 # This is a good approximation if the pixels are big enough | ||
| xmin = np.percentile(self.validation_data[mask], 5) | ||
| xmax = np.percentile(self.validation_data[mask], 95) |
There was a problem hiding this comment.
suggestion for Lines 38-39
xmin, xmax = np.percentile(self.validation_data[mask], [5, 95])
descqa/DensityVersusExtinction.py
Outdated
| Auxiliary function to generate HEALPix maps from catalogs. | ||
| It reads the ra and dec in degrees and returns a HEALPix map | ||
| """ | ||
| pixnums = hp.ang2pix(nside, np.pi/2-np.radians(dec), np.radians(ra)) |
There was a problem hiding this comment.
pixnums = hp.ang2pix(nside, ra, dec, lonlat=True)
descqa/DensityVersusExtinction.py
Outdated
| pixnums = hp.ang2pix(nside, np.pi/2-np.radians(dec), np.radians(ra)) | ||
| return np.bincount(pixnums, minlength=hp.nside2npix(nside)) | ||
|
|
||
| class DensityVersusExtinction(BaseValidationTest): |
There was a problem hiding this comment.
Will this class better be named as DensityVersusSkyPosition or something along the line? In particular, since validation_map_filename and xlabel are both configurable options, one can supply another validation map that is not related to extinction, right? Then it seems to me that the class name is too restrictive?
There was a problem hiding this comment.
You are totally right. Thanks!
There was a problem hiding this comment.
@fjaviersanchez, thanks for the update. One more minor change!
descqa/DensityVersusSkyPosition.py
Outdated
| pixnums = hp.ang2pix(nside, np.pi/2-np.radians(dec), np.radians(ra)) | ||
| return np.bincount(pixnums, minlength=hp.nside2npix(nside)) | ||
| pixnums = hp.ang2pix(nside, ra, dec, lonlat=True) | ||
| return 1.0*np.bincount(pixnums, minlength=hp.nside2npix(nside)) |
There was a problem hiding this comment.
to make it more explicit about the intend
return np.bincount(pixnums, minlength=hp.nside2npix(nside)).astype(np.float)
There was a problem hiding this comment.
thanks, @fjaviersanchez. Should I wait for @rmjarvis to comment before merging?
descqa/DensityVersusSkyPosition.py
Outdated
|
|
||
| class DensityVersusSkyPosition(BaseValidationTest): | ||
| """ | ||
| This tests checks the object density as a function |
descqa/DensityVersusSkyPosition.py
Outdated
| This tests checks the object density as a function | ||
| of another map (e.g: extinction, airmass, etc) | ||
| """ | ||
| def __init__(self,**kwargs): # pylint: disable=W0231 |
There was a problem hiding this comment.
Why use **kwargs here rather than list the kwargs? It looks like the are all required, so normally that would be good to list explicitly in the function declaration. Also worth mentioning what the parameters are in the doc string.
There was a problem hiding this comment.
I did this to be able to "recycle" this script with a different configuration file (and to match the style of most of the tests in descqa). @yymao is there an easy way to use positional arguments and be able to read from different config files?
There was a problem hiding this comment.
@fjaviersanchez I'm not quite sure what you mean "use positional arguments and be able to read from different config files." It is OK to use positional arguments in DESCQA tests; the only requirement is that you must allow additional keyword arguments at the end. See this test for an example. That being said, since these test classes are rarely being called by users in interactive environments, I'm ok with just using **kwargs. The more important thing is to specify the arguments in docstring which I see you already did.
There was a problem hiding this comment.
@fjaviersanchez thanks for the updates. One more small issue. The class is now called DensityVersusSkyPosition as it can accept different validation data, but I think the config yaml file should be named something like DensityVersusExtinction.yaml because this particular config is calling the DensityVersusSkyPosition class to plot Density vs. Extinction, and hence the config itself refers to a specialized test. Does this make sense?
This PR tries to cover one of the tests that are in #123 and solves #134. I am selecting the galaxies from the DM object catalogs using their
extendednessvalues (in particular I requireextendedness==1). I have an example here: https://portal.nersc.gov/project/lsst/descqa/v2/?run=2018-08-14_10&test=DensityVersusExtinction&right=2018-08/2018-08-14_10/DensityVersusExtinction/dc2_coadd_run1.1p/dc2_coadd_run1.1p_density_vs_extinction.png@rmjarvis Right now everything is very vanilla and I assumed that there's no depth variation but no cuts have been made to ensure this. Should I include something to do this?