msimet / Stile

Stile: the Systematics Tests In Lensing pipeline
BSD 3-Clause "New" or "Revised" License
9 stars 6 forks source link

#43 write outputs for displayQA, add rho1, and make it work with the latest reductions #62

Closed HironaoMiyatake closed 9 years ago

HironaoMiyatake commented 9 years ago

Sorry that I ignored this issue for a while. There will be several things to be improved to have Stile work with displayQA more nicely (now I can generate decent displayQA with this pull request, though), but I would like to make a pull request now since I already made a bunch of changes.

msimet commented 9 years ago

I just noticed a kind of silly thing: I think I was calling these Rowe statistics (after @barnabytprowe) not rho statistics. @rmandelb, I know we've discussed this before--did we ever come to a conclusion about which thing people meant?

Otherwise, cool! I'll try to look this over soon, thanks.

rmandelb commented 9 years ago

Well, they are defined in a single-author paper by Barney. And for example this paper describes them as "Rowe" statistics. But I've seen others who wrote rho. So basically, I have no clue. Sorry, I'm not helpful at all.

barnabytprowe commented 9 years ago

Call them rho statistics :) It's a pun anyway, so should always be preferred over something more pious, and I'm happier with a greek letter getting assigned given that all the paper discussed was something so simple as a auto- and cross correlations constructed from modelling residuals!

barnabytprowe commented 9 years ago

(Sorry, wouldn't have seen this but for the ping ;)

msimet commented 9 years ago

Well, I pinged you for a reason. :) I was actually just coming over to say I'd found an old email where you said you didn't like that name so we should stick with rho. So I think that's the executive decision, then.

msimet commented 9 years ago

Hey @HironaoMiyatake -- this looks good; I've left a couple of comments but they shouldn't affect functionality. I also made a commit updating a bit of the wording in the documentation (4a4743b).

I merged master back into this branch, too. The only change you'll need to make, I think, is that if we really do need to keep the individual flags arguments in the different Config objects, they'll need to be changed to flags_keep_false.

HironaoMiyatake commented 9 years ago

@msimet , thank you for reviewing the code! I went through your comments. I made two additional changes;

Please let me know if you are happy with current version. Then I'll merge this into master.

msimet commented 9 years ago

Looks good to me, feel free to merge!

HironaoMiyatake commented 9 years ago

I just merged. I'll look into PR#45 then!