Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-3940

NaiveDipoleCentroid/NaiveDipoleFlux algorithms should not require centroid slot

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ip_diffim
    • Labels:
      None

      Description

      The NaiveDipoleCentroid and NaiveDipoleFlux algorithms in ip_diffim have members which are instances of meas::base::SafeCentroidExtractor. Due to the prerequisites that imposes, it is impossible to initialize these algorithms without first defining a centroid slot.

      However, there is nothing in these algorithms which actually uses the SafeCentroidExtractor or any of the information stored in the slot; this seems to be an entirely arbitrary restriction which is likely a legacy of the port to the meas_base framework. We should remove the use of SafeCentroidExtractor to simply the code and make it easier to run the test suite (since it will no longer be necessary to run a centroider).

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Adding Perry Gee as a watcher, since I believe he did the original port of this code to meas_base. Perry, do you have any insight as to whether SafeCentroidExtractor is actually required here?

            Show
            swinbank John Swinbank added a comment - Adding Perry Gee as a watcher, since I believe he did the original port of this code to meas_base . Perry, do you have any insight as to whether SafeCentroidExtractor is actually required here?
            Hide
            swinbank John Swinbank added a comment -
            Show
            swinbank John Swinbank added a comment - Work is on tickets/DM-3940 in ip_diffim .
            Hide
            pgee Perry Gee added a comment -

            I don't think that I had any particular reason to think that these algorithms required a Centroid. If they pass their tests without one, that seems fine. At the time this port was done, the goal was just to get the code moved over so that it could be worked on in the meas_base environment. I suppose for some reason I thought that it needed a Centroid.

            I do wonder about the addition of "ip_diffim_NaiveDipoleCentroid_pos" as a centroid, and its later removal. Was this also to provide a centroid which wasn't really needed?

            Show
            pgee Perry Gee added a comment - I don't think that I had any particular reason to think that these algorithms required a Centroid. If they pass their tests without one, that seems fine. At the time this port was done, the goal was just to get the code moved over so that it could be worked on in the meas_base environment. I suppose for some reason I thought that it needed a Centroid. I do wonder about the addition of "ip_diffim_NaiveDipoleCentroid_pos" as a centroid, and its later removal. Was this also to provide a centroid which wasn't really needed?
            Hide
            swinbank John Swinbank added a comment -

            Thanks for the review.

            I do wonder about the addition of "ip_diffim_NaiveDipoleCentroid_pos" as a centroid, and its later removal. Was this also to provide a centroid which wasn't really needed?

            Before this issue, it was impossible to use these algorithms without providing a centroid of some sort, even though they didn't make use of it; ip_diffim_NaiveDipoleCentroid_pos seems as good a choice as any.

            Show
            swinbank John Swinbank added a comment - Thanks for the review. I do wonder about the addition of "ip_diffim_NaiveDipoleCentroid_pos" as a centroid, and its later removal. Was this also to provide a centroid which wasn't really needed? Before this issue, it was impossible to use these algorithms without providing a centroid of some sort, even though they didn't make use of it; ip_diffim_NaiveDipoleCentroid_pos seems as good a choice as any.

              People

              • Assignee:
                swinbank John Swinbank
                Reporter:
                swinbank John Swinbank
                Reviewers:
                Perry Gee
                Watchers:
                John Swinbank, Perry Gee, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: