search

LSSTDESC / DC2-analysis

General analysis tools for the DC2 Data Set.
http://lsstdesc.org/DC2-analysis/
BSD 3-Clause "New" or "Revised" License
24 stars 15 forks source link

Add contributed notebook with added functionality beyond FOF matching #75

Closed kakoon closed 5 years ago

kakoon commented 5 years ago

adding mlz input catalog making codes to the fof matching for de-blending

rmandelb commented 5 years ago

Hi @kakoon - I see this Pull Request is for a tutorial notebook - thank you! Can you please clarify how this relates to the tutorial notebook on which it is based (with a similar name) - is the idea that you are suggesting an update to the existing notebook, which could be updated in-place? Or did you intend this as a sufficiently different analysis that it could stand on its own as a new notebook? The answer to this question will determine the next steps for this PR -

kakoon commented 5 years ago

Hi Rachel, it's using fof_matching (similar name notebook) to produce input catalog for MLZ running to calibrate photo-z. It will be used for pz-blending project, that's why I added new notebook instead of adding the part of codes to original fof notebook because if anyone wants simple fof matching only, they won't need part for MLZ.

Best, HyeYun Park

Stony Brook University Physics and Astronomy Brookhaven National Laboratory Large Synoptic Survey telescope Dark Energy Science Collaboration Office: (631) 344-4060 Mobile: (631) 605-0038

On Fri, Mar 22, 2019 at 9:12 AM Rachel Mandelbaum notifications@github.com wrote:

Hi @kakoon https://github.com/kakoon - I see this Pull Request is for a tutorial notebook - thank you! Can you please clarify how this relates to the tutorial notebook on which it is based (with a similar name) - is the idea that you are suggesting an update to the existing notebook, which could be updated in-place? Or did you intend this as a sufficiently different analysis that it could stand on its own as a new notebook? The answer to this question will determine the next steps for this PR -

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LSSTDESC/DC2-analysis/pull/75#issuecomment-475615423, or mute the thread https://github.com/notifications/unsubscribe-auth/AMWwFN-0rlXsTP9zSZ2cr-_HhUunGdvJks5vZNamgaJpZM4b4jaU .

rmandelb commented 5 years ago

Hi @kakoon - thanks for explaining! I think that before this can be reviewed, some clean-up will be needed. The reason is that if I look at the new notebook, it has all the same material at the top as the original one (e.g., it says the same notebook title/topic as the matching_fof.ipynb does, it lists the same authors and learning objectives as that one, same "last verified to run" date, etc.). Here is my suggestion:

Does that make sense? I think this will be a good way to add your new material in a way that others can understand/use while giving attribution to the original tutorial authors for the tutorial this is based on - but I'm also open to other suggestions!

kakoon commented 5 years ago

Hi! yes it makes sense! thank you! I will edit them. Best, HyeYun Park

Stony Brook University Physics and Astronomy Brookhaven National Laboratory Large Synoptic Survey telescope Dark Energy Science Collaboration Office: (631) 344-4060 Mobile: (631) 605-0038

On Mon, Mar 25, 2019 at 10:32 PM Rachel Mandelbaum notifications@github.com wrote:

Hi @kakoon https://github.com/kakoon - thanks for explaining! I think that before this can be reviewed, some clean-up will be needed. The reason is that if I look at the new notebook, it has all the same material at the top as the original one (e.g., it says the same notebook title/topic as the matching_fof.ipynb does, it lists the same authors and learning objectives as that one, same "last verified to run" date, etc.). Here is my suggestion:

-

You can move your notebook into contributed/ (so there will be less of a burden to maintain it in the long term if you don't want to have to keep it up to date in the way that we do with tutorials).

You can update the header to indicate that you developed this based on the matching_fof.ipynb tutorial, and say which material is new/added by you.

Probably should also give the notebook a descriptive name, e.g., matching_fof_blending.ipynb.

Does that make sense? I think this will be a good way to add your new material in a way that others can understand/use while giving attribution to the original tutorial authors for the tutorial this is based on - but I'm also open to other suggestions!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LSSTDESC/DC2-analysis/pull/75#issuecomment-476450575, or mute the thread https://github.com/notifications/unsubscribe-auth/AMWwFGKGLSi6ltyK8yxUOOmCQtDXOxgoks5vaYbMgaJpZM4b4jaU .

kakoon commented 5 years ago

I think I made change and pushed again. Could you check?

Best, HyeYun Park

Stony Brook University Physics and Astronomy Brookhaven National Laboratory Large Synoptic Survey telescope Dark Energy Science Collaboration Office: (631) 344-4060 Mobile: (631) 605-0038

On Mon, Mar 25, 2019 at 10:32 PM Rachel Mandelbaum notifications@github.com wrote:

Hi @kakoon https://github.com/kakoon - thanks for explaining! I think that before this can be reviewed, some clean-up will be needed. The reason is that if I look at the new notebook, it has all the same material at the top as the original one (e.g., it says the same notebook title/topic as the matching_fof.ipynb does, it lists the same authors and learning objectives as that one, same "last verified to run" date, etc.). Here is my suggestion:

-

You can move your notebook into contributed/ (so there will be less of a burden to maintain it in the long term if you don't want to have to keep it up to date in the way that we do with tutorials).

You can update the header to indicate that you developed this based on the matching_fof.ipynb tutorial, and say which material is new/added by you.

Probably should also give the notebook a descriptive name, e.g., matching_fof_blending.ipynb.

Does that make sense? I think this will be a good way to add your new material in a way that others can understand/use while giving attribution to the original tutorial authors for the tutorial this is based on - but I'm also open to other suggestions!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LSSTDESC/DC2-analysis/pull/75#issuecomment-476450575, or mute the thread https://github.com/notifications/unsubscribe-auth/AMWwFGKGLSi6ltyK8yxUOOmCQtDXOxgoks5vaYbMgaJpZM4b4jaU .

rmandelb commented 5 years ago

The latest commit on your branch that I can see is from 10 days ago, so please check - it seems the commits may not have been pushed?

rmandelb commented 5 years ago

Oh - it looks like you created a new branch instead of updating the existing one. I will change the PR to be from the new branch.

rmandelb commented 5 years ago

Never mind, I can't change the originating branch, only the target one. We can close this PR and open a new one from the new branch when you're ready.

rmandelb commented 5 years ago

Closing #75, will open PR from new branch.