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.

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

DM-42814: Cleanup makeKernel candidateList handling #295

Merged
merged 8 commits into from Feb 21, 2024
Merged

Conversation

parejkoj
Copy link
Contributor

No description provided.

Copy link
Contributor

@BrunoSanchez BrunoSanchez left a comment

Choose a reason for hiding this comment

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

This is great.
Many many changes, and more precisely, many code deletions.

I wonder if this can just be merged with a review like this or given the size of the PR, make it open to more people to comment.

@@ -38,6 +40,8 @@
from . import diffimLib


@deprecated(reason="This config class is no longer used. Will be removed after v27.",
version="v27.0", category=FutureWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this showing up in the docs and also during runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will appear any time someone instantiates this class, including in the logs of pipelines that use it.

But this reminded me that I need to also deprecate the ConfigField that uses it in PsfMatchConfig.

@@ -8,7 +8,7 @@
*
* @ingroup ip_diffim
*/

#include <stdexcept>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the LSST exception module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, <> includes are from the standard library. It was necessary for std::logic_error.

Copy link
Contributor

Choose a reason for hiding this comment

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

It all looks cleaner. Thanks

python/lsst/ip/diffim/makeKernel.py Show resolved Hide resolved
@parejkoj
Copy link
Contributor Author

Looking at this again, I think I will have to file an RFC, even though it seems silly: any deprecations do officially need one.

Remove sourceToFootprintList and replace with modern afw Exposure methods:
* makeCandidateList now just takes a SourceCatalog and checks that it is
useable.
* _buildCellSet now takes a SourceCatalog and makes candidates from it
directly.

Cleanup docstrings to match.
An exception here gets logged above anyway, so this is just clutter.
With the refactor of makeCandidateList, the FindSetBits code is no longer
necessary (afw Mask already has everything we need) and
KernelCandidateDetection is ancient and unused.
Since that C++ is the only thing that used it, deprecate DetectionConfig, too.
No need to output such sub-pixel precision.
Move makePoissonNoiseImage from diffimTools.py to the one test that
still uses it.
@parejkoj parejkoj merged commit c18e0e7 into main Feb 21, 2024
2 checks passed
@parejkoj parejkoj deleted the tickets/DM-42814 branch February 21, 2024 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants