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

Refactor DipoleMeasurement: Dipole classification to plugin

    XMLWordPrintable

    Details

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

      Description

      DipoleMeasurementTask currently runs dipole measurement plugins and runs its own implemented dipole classification method. This ticket translates dipole classification to a plugin itself to simplify DipoleMeasurementTask. Instead of inheriting from SingleFrameMeasurementTask, DipoleMeasurementTask will run SingleFrameMeasurementTask setting the appropriate default slots and plugins (including dipole classification plugin).

        Attachments

          Issue Links

            Activity

            Hide
            yusra Yusra AlSayyad added a comment -

            Colin do you have time for a review (ip_diffim, pipe_tasks)?

            Summary of changes:
            DipoleMeasurementTask was just a default configuration + some documentation. There were two options for cleaning this up: make a very light DipoleMeasurementTask which inherited from pipe.base.Task consisting of a config and some documentation, or just delete it (and move the config + docs to ImageDifferenceTask. I chose deleting it, because I envision the processImageDifferenceTask as the reusable task to house the default configs for running SingleFrameMeasurement on image differences and the documentation on what those default plugins are and do. I see this ticket as an intermediate step and moving the config into the current ImageDifferenceTask as a step in the direction we wanted to go. I deleted the docs rather than moving them, because ImageDifferenceTask does not yet have a modern pipe task documentation.

            Note: This changes the field name of the dipole classification from "dipole_classification" to "ip_diffim_ClassificationDipole_value" (and "ip_diffim_ClassificationDipole_flag") analogous to "base_ClassificationExtendedness_value"...

            Show
            yusra Yusra AlSayyad added a comment - Colin do you have time for a review (ip_diffim, pipe_tasks)? Summary of changes: DipoleMeasurementTask was just a default configuration + some documentation. There were two options for cleaning this up: make a very light DipoleMeasurementTask which inherited from pipe.base.Task consisting of a config and some documentation, or just delete it (and move the config + docs to ImageDifferenceTask . I chose deleting it, because I envision the processImageDifferenceTask as the reusable task to house the default configs for running SingleFrameMeasurement on image differences and the documentation on what those default plugins are and do. I see this ticket as an intermediate step and moving the config into the current ImageDifferenceTask as a step in the direction we wanted to go. I deleted the docs rather than moving them, because ImageDifferenceTask does not yet have a modern pipe task documentation. Note: This changes the field name of the dipole classification from "dipole_classification" to "ip_diffim_ClassificationDipole_value" (and "ip_diffim_ClassificationDipole_flag") analogous to "base_ClassificationExtendedness_value"...
            Hide
            ctslater Colin Slater added a comment -

            I noticed one algorithmic change: the measure() function will call fail() without setting an output value if it does not have both a positive and negative flux. I’m not 100% clear on what the semantics are meant to be when a classification plugin "fails", but my inclination is that in this case it should set the output value to 0. I have less of an opinion on whether this also requires a fail flag (the measurement that it is not a dipole may still be reliable). This is overall perhaps a peculiar case where we are only interested positively identifying dipoles, and there is little practical difference between “we couldn’t tell if this is a dipole” and “we know this is not a dipole”.

            It is unfortunate that we have to duplicate the dipole measurement configuration in tests and examples, but I don’t see an easy way around that.

            Everything else seems like the the right thing to do. I concur on the docs decision.

            Show
            ctslater Colin Slater added a comment - I noticed one algorithmic change: the measure() function will call fail() without setting an output value if it does not have both a positive and negative flux. I’m not 100% clear on what the semantics are meant to be when a classification plugin "fails", but my inclination is that in this case it should set the output value to 0. I have less of an opinion on whether this also requires a fail flag (the measurement that it is not a dipole may still be reliable). This is overall perhaps a peculiar case where we are only interested positively identifying dipoles, and there is little practical difference between “we couldn’t tell if this is a dipole” and “we know this is not a dipole”. It is unfortunate that we have to duplicate the dipole measurement configuration in tests and examples, but I don’t see an easy way around that. Everything else seems like the the right thing to do. I concur on the docs decision.
            Hide
            yusra Yusra AlSayyad added a comment -

            Your desired behavior for the classification to be set to 0 by default was being achieved: Because fail() is being called on purpose rather than performed after an exception, the block of code that checks the dipole-conditions are still run and the value is set. In the case where _pos or _neg are nan: nan < anything will evaluate to False and classification=0. In the case where _pos or _neg flags are set, the conditions will be evaluated and classification will be set to 0 or 1. I added a couple comments to make this control flow more clear.

            What is new here is the classification flag. You're right that by calling fail() to set the flag if the _pos/_neg fluxes are NaN, it is being hyperactively set. In response to your review, I changed the logic so that the flag is manually set only if the _pos or _neg flags are set, and not if the _pos/_neg are nan. Of course if there is an exception, it also gets set on its way out and the classification value will be NaN.)

            Yes, having to duplicate the configuration is unfortunate. David Reiss, this probably affects you most immediately as you are editing and adding dipole plugins. Would you take a look before I merge to master?

            Show
            yusra Yusra AlSayyad added a comment - Your desired behavior for the classification to be set to 0 by default was being achieved: Because fail() is being called on purpose rather than performed after an exception, the block of code that checks the dipole-conditions are still run and the value is set. In the case where _pos or _neg are nan: nan < anything will evaluate to False and classification=0. In the case where _pos or _neg flags are set, the conditions will be evaluated and classification will be set to 0 or 1. I added a couple comments to make this control flow more clear. What is new here is the classification flag. You're right that by calling fail() to set the flag if the _pos/_neg fluxes are NaN, it is being hyperactively set. In response to your review, I changed the logic so that the flag is manually set only if the _pos or _neg flags are set, and not if the _pos/_neg are nan. Of course if there is an exception, it also gets set on its way out and the classification value will be NaN.) Yes, having to duplicate the configuration is unfortunate. David Reiss , this probably affects you most immediately as you are editing and adding dipole plugins. Would you take a look before I merge to master?
            Hide
            yusra Yusra AlSayyad added a comment -

            David was also not excited about the code duplication. I added a helper function that makes a default config with the typical dipole measurement config as a new commit. Since the last commit is unreviewed, would @ctslater or one of the watchers take a peak at the last commit in both pipe_tasks and ip_diffim?

            Show
            yusra Yusra AlSayyad added a comment - David was also not excited about the code duplication. I added a helper function that makes a default config with the typical dipole measurement config as a new commit. Since the last commit is unreviewed, would @ctslater or one of the watchers take a peak at the last commit in both pipe_tasks and ip_diffim?
            Hide
            rowen Russell Owen added a comment -

            Can you please explain why you created the makeDipoleMeasurementConfig function instead of subclassing SingleFrameMeasurementConfig and defining the desired defaults using the setDefaults method? I see that your function takes a config as an argument, and I suspect that's key, but I didn't spot any code using that method. I suppose it's not a traditional use case anyway, since you are adding a plugin instead of creating a new task, so a function is probably as good as a new config class.

            Show
            rowen Russell Owen added a comment - Can you please explain why you created the makeDipoleMeasurementConfig function instead of subclassing SingleFrameMeasurementConfig and defining the desired defaults using the setDefaults method? I see that your function takes a config as an argument, and I suspect that's key, but I didn't spot any code using that method. I suppose it's not a traditional use case anyway, since you are adding a plugin instead of creating a new task, so a function is probably as good as a new config class.
            Hide
            yusra Yusra AlSayyad added a comment -

            I'd like to close this. I put a new version on u/yusra/DM-3515 that does not remove the old DipoleMeasurementTask and does not touch the imageDifference.py driver in pipe_tasks.

            This will all be replaced by David Reiss's new DipoleFitTask soon anyway. The only impact is that the name of the slot in which the current dipole classification has been changed from "classification_dipole" to "ip_diffim_ClassificationDipole_value"

            I will merge tomorrow, unless I hear from any of you watchers.

            Show
            yusra Yusra AlSayyad added a comment - I'd like to close this. I put a new version on u/yusra/ DM-3515 that does not remove the old DipoleMeasurementTask and does not touch the imageDifference.py driver in pipe_tasks. This will all be replaced by David Reiss 's new DipoleFitTask soon anyway. The only impact is that the name of the slot in which the current dipole classification has been changed from "classification_dipole" to "ip_diffim_ClassificationDipole_value" I will merge tomorrow, unless I hear from any of you watchers.
            Hide
            reiss David Reiss added a comment -

            I added a few minor comments to a new PR. I was not able to run the tests or examples today, as I have had to rebuild the stack this morning. One thing to check: the testDipoleFitter.py tests that I just merged compare the results to the dipoleMeasurementTask. Can you just verify that those tests still pass?

            Show
            reiss David Reiss added a comment - I added a few minor comments to a new PR. I was not able to run the tests or examples today, as I have had to rebuild the stack this morning. One thing to check: the testDipoleFitter.py tests that I just merged compare the results to the dipoleMeasurementTask. Can you just verify that those tests still pass?

              People

              Assignee:
              yusra Yusra AlSayyad
              Reporter:
              yusra Yusra AlSayyad
              Reviewers:
              Colin Slater
              Watchers:
              Colin Slater, David Reiss, Russell Owen, Simon Krughoff, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: