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.

Sign up for GitHub

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

Jump to bottom

DM-37555: Update pipeline documentation for image differencing #294

Merged
merged 1 commit into from
Mar 4, 2024
Merged

DM-37555: Update pipeline documentation for image differencing #294

merged 1 commit into from
Mar 4, 2024

Conversation

BrunoSanchez
Copy link
Contributor

Big documentation update, removed old docs and added basic help info.

@parejkoj
Copy link
Contributor

Please fix the PR title. See here:
https://developer.lsst.io/work/flow.html#make-a-pull-request

@BrunoSanchez BrunoSanchez changed the title tickets/DM-37555 DM-37555: Update pipeline documentation for image differencing Jan 30, 2024
@BrunoSanchez BrunoSanchez marked this pull request as ready for review January 30, 2024 19:11
parejkoj
parejkoj requested changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion: please use the "one sentence per line" format described here in the overview:

https://developer.lsst.io/restructuredtext/style.html#text-wrapping

instead of breaking your lines at ~80chars. It makes future modifications easier, and makes parsing the raw ReST easier. I'll look at the overview file after it's refactored, so it's easier to comment on.

doc/lsst.ip.diffim/index.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/index.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/index.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/index.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/index.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/tasks/lsst.ip.diffim.AlardLuptonSubtractTask.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/tasks/lsst.ip.diffim.AlardLuptonSubtractTask.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/tasks/lsst.ip.diffim.AlardLuptonSubtractTask.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/tasks/lsst.ip.diffim.DetectAndMeasureTask.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/tasks/lsst.ip.diffim.DetectAndMeasureTask.rst Outdated Show resolved Hide resolved
@BrunoSanchez
Copy link
Contributor Author

OK, all changes implemented.
Sorry I took long to realize this was reviewed already.

isullivan
isullivan approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@isullivan isullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I had a few wording changes.

doc/lsst.ip.diffim/index.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/overview_ipdiffim.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/overview_ipdiffim.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/overview_ipdiffim.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/tasks/lsst.ip.diffim.AlardLuptonPreconvolveSubtractTask.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/tasks/lsst.ip.diffim.DetectAndMeasureScoreTask.rst Outdated Show resolved Hide resolved
doc/lsst.ip.diffim/tasks/lsst.ip.diffim.MakeKernelTask.rst Outdated
and measurement of candidate `~lsst.ip.diffim.KernelCandidates` is defined, and
used to initize subTasks selectDetection (for candidate detection) and
selectMeasurement(for candidate measurement).
Build a Psf-matching kernel using two input images, either as MaskedImages (in which case they need to be astrometrically aligned) or Exposures (in which case astrometric alignment will happen by default but may be turned off).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this Task can handle Exposures that are not astrometrically aligned any more. It was certainly my intention to remove that functionality, so if it is still there we should disable it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look and it seems this functionality is not there anymore. I couldn't find it at least. Changed the docs to reflect this.

@BrunoSanchez
Update documentation for ip.diffim
2d2d4af 
Remove documentation pages with outdated information.
Include new tasks doc pages and fix docstrings.
Add a new overview page for ip.diffim

Adding the examples in the landing page

Fix bug on docstring

Fix review comments

Update doc/lsst.ip.diffim/index.rst

Co-authored-by: Ian Sullivan <sullii@uw.edu>

Update doc/lsst.ip.diffim/overview_ipdiffim.rst

Co-authored-by: Ian Sullivan <sullii@uw.edu>

Fix review comments

Fix review comments
@BrunoSanchez BrunoSanchez merged commit 4a458ce into main Mar 4, 2024
2 checks passed
@BrunoSanchez BrunoSanchez deleted the tickets/DM-37555 branch March 4, 2024 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parejkoj parejkoj parejkoj approved these changes

@isullivan isullivan isullivan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BrunoSanchez @parejkoj @isullivan