drphilmarshall / OM10

Tools for working with the Oguri & Marshall (2010) mock catalog of strong gravitational lenses
MIT License
8 stars 19 forks source link

Final Pull Request #57

Closed jennykim1016 closed 7 years ago

jennykim1016 commented 7 years ago

I have minimal experiences writing the scientific documents, so if the notebook should be improved in some way, please let me know! All the codes and notebooks are the final version. Thanks.

drphilmarshall commented 7 years ago

Thanks Jenny! I'm still a bit confused by the "files changed" though: can you please do a git pull base master into your master branch? Its good to do this to minimize the changes listed. Then, it looks like you have deleted the om10/__init__.py file, and that you still have some odd looking data files lying around (data/1.txt etc).

drphilmarshall commented 7 years ago

Color Comparison notebook looks pretty good, though! :-)

jennykim1016 commented 7 years ago

Oh, yes! Thanks. I am at a meeting right now, but I will do that once I get back to my dorm.

jennykim1016 commented 7 years ago

Sorry that it took a little longer. The git pull base master command says I should check whether I have a correct right access. So, I just did git pull from my master's branch and also from your branch, but the former said already up to date and the latter said updated travis. Those two did not seem to solve the issue, and the commands did not also retrieve the missing files in the OM10/om10 directory. So, I cloned your master OM10 again locally and manually copied the files that the forked OM10 did not have. This solves the issue, but it seems like a rather unintelligent way to work with Git; I will try to make git pull base master work.

drphilmarshall commented 7 years ago

OK, that looks cleaner! You can remove examples/.DS_Store, it's an uninteresting and binary file. In general we should avoid doing git add on whole directories, that's usually how this sort of cruft ends up being mistakenly checked in. I'll take a look at your notebook now.

drphilmarshall commented 7 years ago

@jennykim1016

Hi Jenny! I pulled your commits into the issue/43/painting branch and tried running the Color Comparison notebook. As you can see, I found a bug: the magnitudes are not being referenced correctly. Can you fix this please, and make sure (by doing a "Restart and Run All Cells") that the notebook works correctly before checking in again? I'll submit a PR to you from the issue branch so you get my version, which has some edits to the text (look at the differences!) and a couple of suggestions for quantifying your argument that the synthetic photometry is good.

jennykim1016 commented 7 years ago

Oh, sorry about that! The link you gave me says 404 page not found, but I will work on fixing the Color Comparison.ipynb in my master directory.

Edit : I found the file from the pull request in my OM10 fork. I will work on fixing that file.

drphilmarshall commented 7 years ago

Hi Jenny - looks like you are getting there, although the presentation of the numbers needs work. You should present numbers that look like $2.1 \pm 0.3$, ie rounded off to appropriate precision (so that the decimal places in the error bar matches that in the estimate), and then compare them as follows: take the difference in the means as $\delta x$. Then compute the error in that difference (approximately) by doing $\sigma = \sqrt{\sigma_1^2 + \sigma_2^2}$. The "number of sigma" difference is then $\delta x / \sigma$. If this ratio comes out as 1.8 (for example), you would say "the two colors agree at the 1.8-sigma level". 2-sigma is cause for concern, 3-sigma or more and we go back to the drawing board. You might want to make a markdown cell like this after each one of your plots, to extract a conclusion from each comparison. The final conclusions section would then just pull the three individual results sections together. Sound like a plan?

jennykim1016 commented 7 years ago

Thanks! My computer freezes whenever I try to run the notebook for more than two times, so I committed this so that I could work on a different computer. The notebook I just committed is not definitely the final version. I will finish this shortly and make a real commit!

p.s. Congratulations on the DESC election, I am so delighted for you!!

drphilmarshall commented 7 years ago

:-) Thanks, Jenny!

drphilmarshall commented 7 years ago

Hi @jennykim1016 - I pulled in the OM10 master branch after merging your pull request and was able to run your Color Comparison notebook, after some editing. I needed to remove the hard-coded catalog path, and then edit the plotting code to use the right axis labels. I then found a bug - in the final comparison you were using lens redshift instead of source redshift. I'm still not sure all comparisons are being done correctly - great if you can go carefully through them and make sure that you are comparing things correctly. To help you with this I suggest you do not re-use the data variable, but instead make a unique Table for each dataset you compare to.