Transients #3
Transients #3
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| @@ -0,0 +1,602 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #2. def display_lenses(ra, dec, transient_pos, skymap=None):
do you want to define the definition inputs?
Reply via ReviewNB
| @@ -0,0 +1,602 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #7. skymap = butler.get("skymap")
what is the butler object here? It is not in the definition input and I am not sure it should be defined as a global variable
Reply via ReviewNB
| @@ -0,0 +1,602 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #17. if cutout_g != None:
I am not sure what the logic of this is. I guess when the g band fails also the rest is expected to fail? Perhaps commenting in the code what and why it is doing what it does might be helpful
Reply via ReviewNB
| @@ -0,0 +1,602 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. repo = '/global/cfs/cdirs/lsst/production/gen3/DC2/Run3.1i/repo'
can you also describe in words where and from where this pointer goes to (i.e. in NERSC towards DC2 products etc. These paths might change in the future and knowing to what type of products these point can be useful
Reply via ReviewNB
| @@ -0,0 +1,602 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #27. regions: array
it looks this input must have a specific format. I guess it goes into a class, so perhaps specify for what class this input needs to go and satisfy might be useful
Reply via ReviewNB
| @@ -0,0 +1,602 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. def get_lens_radec(cat, butler, region=None, time=False, skymap=None):
can you describe the input, in particular the 'cat' part since it needs to satisfy a specific format
Reply via ReviewNB
| @@ -0,0 +1,602 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #27. agn_lenses = get_lens_radec(agn, butler, skymap=skymap)
perhaps these two lines and the next two blocks can get merged as they are executions of tasks to get the lenses and produce the coadds etc?
Reply via ReviewNB
| @@ -0,0 +1,602 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. from desclamp import lens_sources as ls
can you describe what happens in this code block? There is a lot going on and it might be useful for the user to have text describing it (at least a bit)
Reply via ReviewNB
| @@ -0,0 +1,602 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #4. from lsst.pipe.tasks.insertFakes import _add_fake_sources
here you import a private definition from another package. If possible, this should be avoided as private definitions might change without deprecation warnings etc. One way to do this is perhaps to ask the list.pipe.tasks.insertFakes developer to also add a public facing add_fake_sources() definition to signal them that this is used and should remain intact.
Reply via ReviewNB
| @@ -0,0 +1,560 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #11. def is_in_region(ra,
this function is also defined in another notebook. I guess these functions can be defined in a package somewhere (either in the SL pipeline stack or somewhere else on the LSST pipeline stack) such that they can be re-used. I understand that you first code them up in this notebook. Would be better for prosperity and to avoid duplications
Reply via ReviewNB
New notebooks on ddf analysis and calexp source injection.