Size luminosity test

[vvinuv]

Is it okay now? I am not quite sure why I could not update the branch 'vinu'?

[yymao]

@vvinuv thanks. This is good.

You can also update the branch vinu, but it is recommended to use a branch for a new task that you are working on (and use corresponding name for the branch).

[yymao]

@vvinuv can you also paste a link to a DESCQA run of this PR? Thanks.

[vvinuv]

@yymao Thanks! Here is the link https://portal.nersc.gov/project/lsst/descqa/v2/?run=2018-01-06_28

…

On 6 January 2018 at 13:47, Yao-Yuan Mao ***@***.***> wrote: @vvinuv <https://github.com/vvinuv> can you also paste a link to a DESCQA run of this PR? Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJ23Y8l_aandz1Z968oilpL0VXff4G5ks5tH83CgaJpZM4RVarj> .

-- Vinu Vikraman http://www.hep.anl.gov/vvikraman/ <http://www.sas.upenn.edu/~vinu/>

[rmandelb]

@vvinuv - I took a look at the plots and have some questions:

• Can you clarify whether the errorbars on the points and the width of the shaded region is meant to capture the scatter in the distribution? Or error on the mean? I assume it's the former but I wonder if there is some way to indicate this.

• Something funny is going on with the tickmarks on the vertical and horizontal axes. It's like there are two sets of tickmarks overlaid, indicating different things - do you see what I mean? (e.g. on the vertical axis, between 1 and 10 kpc there are way more than just the 8 tickmarks you might expect for (2,3,4,5,6,7,8,9) kpc, and they are oddly spaced)

[vvinuv]

@rmandelb Sorry for the late reply.

1. As you have guessed the shaded regions and errors on the points indicate the scatters in the sizes NOT the errors on the mean. Is it okay to label those as the scatters or do you have some other ideas?

2. Sorry about the tickmarks. I think there is an issue in the script on the subplots. Will change that soon.

[vvinuv]

@rmandelb There was a bug and the labels are fixed https://portal.nersc.gov/project/lsst/descqa/v2/?run=2018-01-08_6&test=size_vanderWel2014_SM_Lum

@yymao I think if @rmandelb has any suggestions to label the errors in the figure I will update the script again for PR.

[rmandelb]

Thanks for the bug fix.

Regarding the labels, perhaps you could just update the plot title to say something like "buzzard_test vs. van der Wel+2014: comparing mean and scatter"?

[vvinuv]

Thanks @rmandelb! Here is the output https://portal.nersc.gov/project/lsst/descqa/v2/?run=2018-01-08_8&test=size_vanderWel2014_SM_Lum

[rmandelb]

Looks good to me, thank you.

[yymao]

@vvinuv thanks for updating the PR. Can you post a link to these tests in the web interface?

I think the major change required here is to merge SizeStellarMassLuminosity.py and SizeStellarMassLuminosityBulgeDisk.py. The two modules are very similar and I think they can be merged and we can use the config file to control the behavior.

Also, the tests should refrain from checking which catalog is being run on. The conditional statement in SizeStellarMassLuminosityBulgeDisk.py that checks for buzzard is now not necessary since we also have a test to test total size.

In short, I think we need only one module, and two config files. One config file is for the total size test, and the other is for the disk/bulge size test. And when the disk/bulge option is on, the test would check whether or not per-component sizes exist in the catalog, and skip the catalog if not.

Does that make sense?

[vvinuv]

@yymao it does make sense. I was doing the separate scripts as those use completely different parameters from the catalog. I will check if I can rewrite one validation script.

…

On 21 February 2018 at 10:53, Yao-Yuan Mao ***@***.***> wrote: ***@***.**** requested changes on this pull request. @vvinuv <https://github.com/vvinuv> thanks for updating the PR. Can you post a link to these tests in the web interface? I think the major change required here is to merge SizeStellarMassLuminosity.py and SizeStellarMassLuminosityBulgeDisk.py. The two modules are very similar and I think they can be merged and we can use the config file to control the behavior. Also, the tests should refrain from checking which catalog is being run on. The conditional statement in SizeStellarMassLuminosityBulgeDisk.py that checks for buzzard is now not necessary since we also have a test to test total size. In short, I think we need only one module, and two config files. One config file is for the total size test, and the other is for the disk/bulge size test. And when the disk/bulge option is on, the test would check whether or not per-component sizes exist in the catalog, and skip the catalog if not. Does that make sense? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJ23aQ4yZh6mO_4HkVV1AN259jpcMPgks5tXEobgaJpZM4RVarj> .

-- Vinu Vikraman http://www.hep.anl.gov/vvikraman/ <http://www.sas.upenn.edu/~vinu/>

[yymao]

@vvinuv thanks for the update. Let me know if this is ready to be reviewed.

BTW, is the file SizeAppmagBulgeDiskCOSMOS.py used by any test? Right now I found no config file using it.

[vvinuv]

@yymao Yes, it is ready for review. I will delete SizeAppmagBulgeDiskCOSMOS from the repository

…

On 23 February 2018 at 10:26, Yao-Yuan Mao ***@***.***> wrote: @vvinuv <https://github.com/vvinuv> thanks for the update. Let me know if this is ready to be reviewed. BTW, is the file SizeAppmagBulgeDiskCOSMOS.py used by any test? Right now I found no config file using it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJ23ezsGFTfypC4yh6g1LSVp_DpRYfbks5tXua8gaJpZM4RVarj> .

-- Vinu Vikraman http://www.hep.anl.gov/vvikraman/ <http://www.sas.upenn.edu/~vinu/>

[yymao]

@vvinuv Can you take a look at https://travis-ci.org/LSSTDESC/descqa/jobs/345344135#L600. Lines that are started with W: need to be addressed. Mostly just to remove unused variable.

For the "super-init-not-called" warning, add #pylint: disable=W0231 or super(SizeStellarMassLuminosity, self).__init__(**kwargs), to the __int__ function.

[vvinuv]

@yymao I just did it. I am not quite sure what you meant by "For the "super-init-not-called" warning, add #pylint: disable=W0231 or super(SizeStellarMassLuminosity, self).__init__(**kwargs), to the __int__ function."? Is that referring to "W: 29, 4: __init__ method from base class 'BaseValidationTest' is not called (super-init-not-called)"

…

On 23 February 2018 at 13:45, Yao-Yuan Mao ***@***.***> wrote: @vvinuv <https://github.com/vvinuv> Can you take a look at https://travis-ci.org/LSSTDESC/descqa/jobs/345344135#L600. Lines that are started with W: need to be addressed. Mostly just to remove unused variable. For the "super-init-not-called" warning, add #pylint: disable=W0231 or super(SizeStellarMassLuminosity, self).__init__(**kwargs), to the __int__ function. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJ23ZX6T7iBLKXlNF051NvaF0oVE5u5ks5tXxVsgaJpZM4RVarj> .

-- Vinu Vikraman http://www.hep.anl.gov/vvikraman/ <http://www.sas.upenn.edu/~vinu/>

[yymao]

yes.