DM-35007: Make schema documentation of HSM moments useful #38

arunkannawadi · 2022-06-16T14:52:10Z

No description provided.

parejkoj

Thanks for improving these docs; I have some wording suggestions, based on how these look in the schema you posted on Jira. I'm not wedded to that new wording, but I think we should look at it from the perspective of someone reading the schema.

I do wish we had some discussion about what exactly HSM is. The readme is entirely unhelpful, and we don't have any package docs. I believe the term comes from the name of one or more papers: could you please add a link to the relevant papers to the readme as part of this? That's the minimum: I think some package docs are probably out of scope for this ticket, but please file a ticket linked from this one saying that we need them.

parejkoj · 2022-09-23T21:22:08Z

include/lsst/meas/extensions/shapeHSM/HsmMomentsControl.h

-    HsmSourceMomentsAlgorithm(Control const & ctrl, std::string const & name, afw::table::Schema & schema) :
-        HsmMomentsAlgorithm(name, schema, "Source adaptive moments algorithm from HSM"), _ctrl(ctrl)
+    HsmSourceMomentsAlgorithm(Control const & ctrl, std::string const & name, afw::table::Schema & schema,
+                              char const* doc = "Source adaptive moments algorithm from HSM") :  ///< Docstring is exposed for HsmSourceMomentsRoundAlgorithm


"from HSM" or "from shapeHSM"? Are we referring to the HSM paper here, or to this shapeHSM package?

Seeing how these come out in the schema, I wonder if "Source adaptive moments via the HSM shape algorithm" (still not sure whether to use shapeHSM or HSM shape or what) might be better? A user reading the catalog schema is asking "what is this and where did it come from", and "X algorithm from HSM" is answering a different question.

Forgive me for intruding, but I thought HSM came from "Hirata, Seljak Moments": https://ui.adsabs.harvard.edu/abs/2003MNRAS.343..459H/abstract (adaptive moments). I've heard "Hirata, Seljak, Mandelbaum", but I'm not sure that's the case. I'll ask Rachel Mandelbaum. (This is in response to the comment above "I do wish we had some discussion about what exactly HSM is. ")

I don't think shapeHSM is going to be particularly useful either, since that's literally the string in the column name. HSM, in practice, refers to a very specific implementation of an algorithm that's not fundamentally different from SDSS shapes. I don't have a better alternative to what you suggested John, so I will go with it. I'll add the reference in the README and create a ticket to put in some module level documentation explaining it in a bit more detail.

parejkoj · 2022-09-23T21:23:58Z

include/lsst/meas/extensions/shapeHSM/HsmMomentsControl.h

+public:
+    /// @brief Initialize with standard field names and customized documentation.
+    HsmSourceMomentsRoundAlgorithm(Control const & ctrl, std::string const & name, afw::table::Schema & schema,
+                                   char const* doc = "Source adaptive moments algorithm from HSM, with circular weight function") :


As above, "Source adaptive moments via the HSM shape algorithm..." or something like that.

parejkoj · 2022-09-23T21:24:27Z

include/lsst/meas/extensions/shapeHSM/HsmMomentsControl.h

-        HsmMomentsAlgorithm(name, schema, "Psf adaptive moments algorithm from HSM"), _ctrl(ctrl) {}
-
+    HsmPsfMomentsAlgorithm(Control const & ctrl, std::string const & name, afw::table::Schema & schema,
+                           char const* doc = "PSF adaptive moments algorithm from HSM"  ///< Docstring is exposed for debiased PSF algorithm


As above, "PSF adaptive moments via the HSM shape algorithm"

I can see how this can be confusing as well, if it is interpreted to mean some PSF was adapted to some standard one to measure the moments. I will change this to "Adaptive moments of the PSF via the HSM shape algorithm".

parejkoj · 2022-09-23T21:25:48Z

include/lsst/meas/extensions/shapeHSM/HsmMomentsControl.h

-    HsmPsfMomentsDebiasedAlgorithm(Control const & ctrl, std::string const & name, afw::table::Schema & schema) :
-        HsmPsfMomentsAlgorithm(ctrl, name, schema) {
+    HsmPsfMomentsDebiasedAlgorithm(Control const & ctrl, std::string const & name, afw::table::Schema & schema,
+                                   char const* doc = "Debiased PSF adaptive moments algorithm from HSM") :


As above, "Debiased PSF adaptive moments via the HSM shape algorithm", or similar.

and this to "Debiased adaptive moments of the PSF via the HSM shape algorithm".

parejkoj · 2022-09-23T21:26:50Z

python/lsst/meas/extensions/shapeHSM/__init__.py

@@ -39,7 +39,7 @@
                    Control=HsmShapeRegaussControl, executionOrder=BasePlugin.SHAPE_ORDER)
 wrapSimpleAlgorithm(HsmSourceMomentsAlgorithm, name="ext_shapeHSM_HsmSourceMoments",
                    Control=HsmSourceMomentsControl, executionOrder=BasePlugin.SHAPE_ORDER)
-wrapSimpleAlgorithm(HsmSourceMomentsAlgorithm, name="ext_shapeHSM_HsmSourceMomentsRound",
+wrapSimpleAlgorithm(HsmSourceMomentsRoundAlgorithm, name="ext_shapeHSM_HsmSourceMomentsRound",


Oh, that's definitely a bug! Good catch.

FYI: It wasn't a bug, since whether round moments are calculated or not, was entiredly determined with the Control class. HsmSourceMomentsRoundAlgorithm is defined in this ticket, and is subclassed solely to have a different docstring.

arunkannawadi · 2022-09-23T21:44:03Z

Yup, you're correct Andres. That's the origin of HSM.

…

On Fri, Sep 23, 2022, 5:37 PM Andrés Alejandro Plazas Malagón < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/lsst/meas/extensions/shapeHSM/HsmMomentsControl.h <#38 (comment)> : > @@ -183,8 +183,9 @@ class HsmSourceMomentsAlgorithm : public HsmMomentsAlgorithm { typedef HsmSourceMomentsControl Control; /// @brief Initialize with standard field names and customized documentation. - HsmSourceMomentsAlgorithm(Control const & ctrl, std::string const & name, afw::table::Schema & schema) : - HsmMomentsAlgorithm(name, schema, "Source adaptive moments algorithm from HSM"), _ctrl(ctrl) + HsmSourceMomentsAlgorithm(Control const & ctrl, std::string const & name, afw::table::Schema & schema, + char const* doc = "Source adaptive moments algorithm from HSM") : ///< Docstring is exposed for HsmSourceMomentsRoundAlgorithm Forgive me for intruding, but I thought HSM came from "Hirata, Seljak Moments": https://ui.adsabs.harvard.edu/abs/2003MNRAS.343..459H/abstract (adaptive moments). I've heard "Hirata, Seljak, Mandelbaum", but I'm not sure that's the case. I'll ask Rachel Mandelbaum. (This is in response to the comment above "I do wish we had some discussion about what exactly HSM is. ") — Reply to this email directly, view it on GitHub <#38 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABG56HURKIIKA6VSCT5B4ULV7YPLBANCNFSM5Y7CPCCA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

plazas · 2022-09-23T22:20:25Z

So, it turns out that I was wrong, and it is indeed "Hirata, Seljak, and Mandelbaum" :) I'm glad I asked Rachel. She said that Hirata & Seljak developed the formalism and initial code, but then she modified it when applying it in practice to the Mandelbaum et al (2005) SDSS shape catalog paper. So it’s Hirata, Seljak, and Mandelbaum because they initiated the concept and code and she made updates to get it working in practice on the data.

arunkannawadi · 2022-09-23T23:38:26Z

Oh, I misread the exact wording of your original comment. I did mean HSM stands for Hirata, Seljak & Mandelbaum but good that you double checked that

…

On Fri, Sep 23, 2022, 6:20 PM Andrés Alejandro Plazas Malagón < ***@***.***> wrote: So, it turns out that I was wrong, and it is indeed "Hirata, Seljak, and Mandelbaum" :) I'm glad I asked Rachel. She said that Hirata & Seljak developed the formalism and initial code, but then she modified it when applying it in practice to the Mandelbaum et al (2005) SDSS shape catalog paper. So it’s Hirata, Seljak, and Mandelbaum because they initiated the concept and code and she made updates to get it working in practice on the data. — Reply to this email directly, view it on GitHub <#38 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABG56HR2LIIHQBWVEWSMGSTV7YULHANCNFSM5Y7CPCCA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

parejkoj

Thank you, that info in the readme is a good start to describing what this does.

parejkoj requested changes Sep 23, 2022

View reviewed changes

arunkannawadi force-pushed the tickets/DM-35007 branch from 983f8fc to 6dd8777 Compare October 3, 2022 18:06

parejkoj approved these changes Oct 3, 2022

View reviewed changes

arunkannawadi added 6 commits October 4, 2022 06:27

Remove hardcoded field docs

30f8c2d

Add doc as an optional argument

b12b318

Improve the wording of the exposed doc

e9e9df0

Make HsmSourceMomentsRound its own algorithm class

009e2a5

Add schema documentation to unit tests

44e0b7b

Improve README

a7b4876

arunkannawadi force-pushed the tickets/DM-35007 branch from df98a80 to a7b4876 Compare October 4, 2022 13:28

arunkannawadi merged commit 9e40408 into main Oct 4, 2022

arunkannawadi deleted the tickets/DM-35007 branch October 4, 2022 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-35007: Make schema documentation of HSM moments useful #38

DM-35007: Make schema documentation of HSM moments useful #38

arunkannawadi commented Jun 16, 2022

parejkoj left a comment

parejkoj Sep 23, 2022

plazas Sep 23, 2022

arunkannawadi Oct 3, 2022

parejkoj Sep 23, 2022

parejkoj Sep 23, 2022

arunkannawadi Oct 3, 2022

parejkoj Sep 23, 2022

arunkannawadi Oct 3, 2022

parejkoj Sep 23, 2022

arunkannawadi Oct 3, 2022

arunkannawadi commented Sep 23, 2022 via email

plazas commented Sep 23, 2022

arunkannawadi commented Sep 23, 2022 via email

parejkoj left a comment

DM-35007: Make schema documentation of HSM moments useful #38

DM-35007: Make schema documentation of HSM moments useful #38

Conversation

arunkannawadi commented Jun 16, 2022

parejkoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkannawadi commented Sep 23, 2022 via email

plazas commented Sep 23, 2022

arunkannawadi commented Sep 23, 2022 via email

parejkoj left a comment

Choose a reason for hiding this comment