Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Self-documenting variable names? #28

Open
rmandelb opened this issue Aug 8, 2017 · 3 comments
Open

Self-documenting variable names? #28

rmandelb opened this issue Aug 8, 2017 · 3 comments

Comments

@rmandelb
Copy link
Contributor

rmandelb commented Aug 8, 2017

Hi @dkirkby -

My student Husni ( @hsnee ) has been looking through your code to learn how to use it for his DESC work. While doing so, we found something a bit confusing:

You have some variables in the output tables with names like psf_X (for example, psf_sigm) which seem like they are meant to include properties of the PSF. But if we look at where this quantity is set, https://github.com/LSSTDESC/WeakLensingDeblending/blob/master/descwl/analysis.py#L917, it appears that this is actually a property of the galaxy after PSF convolution. This is certainly a minor point, but it might make the outputs a bit more self-documenting if the column names and/or variable names are a bit more clear about what this quantity means.

@dkirkby
Copy link
Member

dkirkby commented Aug 9, 2017

I agree that variable names could be generally improved here and any suggestions are welcome! Keep in mind that you often end up typing these names when exploring the outputs interactively in a jupyter notebook (without cmd-line completion on table column names).

Also, since you mentioned digging in the code to figure out what psf_sigm does, perhaps you didn't already see this page on the documentation web site:

http://weaklensingdeblending.readthedocs.io/en/latest/output.html

psf_sigm is described there as:

PSF-convolved half-light radius in arcseconds calculated as |Q|**0.25

@rmandelb
Copy link
Contributor Author

Hi @dkirkby -

Keep in mind that you often end up typing these names when exploring the outputs interactively
in a jupyter notebook (without cmd-line completion on table column names).

Good point. What about something like gal_sigm and gal_obssigm for the pre-seeing and observed size?

We did see your (excellent and much-appreciated) documentation, but I saw psf_sigm in two places when I searched on that page. In the first place, it's in the section defining survey parameters, and is defined as PSF size \|Q\|**0.25 in arcsecs (unweighted Q). Then later there's the definition that you gave in this issue, PSF-convolved half-light radius in arcseconds calculated as |Q|**0.25. I realize now that the first place is referring to survey parameters in the header and the second is referring to a catalog entry, but I still found it confusing in my initial look that the same name is used for two different things.

@dkirkby
Copy link
Member

dkirkby commented Aug 11, 2017

Good point, that is confusing! Let's change the catalog name as you proposed.

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

No branches or pull requests

2 participants