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

Enable aperture correction in the integration test

    Details

      Description

      The present integration test does not enable aperture correction. This should be enabled and the results sanity-checked.

      This is a separate ticket rather than DM-436 at Jim Bosch's suggestion, to avoid ticket bloat.

      It requires two separate changes:

      • update obs_sdss's SdssCalibrateTask to measure and apply aperture correction
      • update the expected results from the integration test lsst_dm_stack_demo

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            I added an allowApCorr flag to SingleFrameMeasurementTask.run (which defaults to True) to support SdssCalibrateTask, because it calls measurement.run several times, an ap corr data is only available for the final call. I did the same for ForcedMeasurementTask for symmetry. SdssCalibrateTask used to work without this until I made failure to apply aperture correction an exception. This new argument adds a bit of duplication with the config parameter doApplyApCorr, but I suggest we live with it. The duplication is mostly evident when calling initialMeasurement in [Sdss]CalibrateTask, because the corresponding doApplyApCorr parameter is ignored (initialMeasurement never has ap corr data); I set the parameter to "no" in setDefaults to avoid confusing configs, but it's not necessary.

            Another possible approach is an argument doAllowApCorr that overrides the config parameter if not None. That would be more flexible, but I have rejected it for now because it is easier to explain why the flag is needed than a full override. We have no use case for a full override and I'm uncomfortable with too many ways to do something. Also, as a minor point, it is potentially a pain to validate the argument, since it is a keyword (though pex_config ChoiceField has a private method _validateValue(value) that would work, and it could be made public if we have enough need).

            Show
            rowen Russell Owen added a comment - I added an allowApCorr flag to SingleFrameMeasurementTask.run (which defaults to True) to support SdssCalibrateTask, because it calls measurement.run several times, an ap corr data is only available for the final call. I did the same for ForcedMeasurementTask for symmetry. SdssCalibrateTask used to work without this until I made failure to apply aperture correction an exception. This new argument adds a bit of duplication with the config parameter doApplyApCorr, but I suggest we live with it. The duplication is mostly evident when calling initialMeasurement in [Sdss] CalibrateTask, because the corresponding doApplyApCorr parameter is ignored (initialMeasurement never has ap corr data); I set the parameter to "no" in setDefaults to avoid confusing configs, but it's not necessary. Another possible approach is an argument doAllowApCorr that overrides the config parameter if not None. That would be more flexible, but I have rejected it for now because it is easier to explain why the flag is needed than a full override. We have no use case for a full override and I'm uncomfortable with too many ways to do something. Also, as a minor point, it is potentially a pain to validate the argument, since it is a keyword (though pex_config ChoiceField has a private method _validateValue(value) that would work, and it could be made public if we have enough need).
            Hide
            rowen Russell Owen added a comment -

            Merged minor changes in meas_base, pipe_tasks, obs_sdss and a new detected-sources_small.txt.expected in obs_sdss

            Show
            rowen Russell Owen added a comment - Merged minor changes in meas_base, pipe_tasks, obs_sdss and a new detected-sources_small.txt.expected in obs_sdss
            Hide
            swinbank John Swinbank added a comment -

            It looks like we've only got a new detected-sources_small.txt.expected and not a new detected-sources.txt.expected, which seems surprising. Have I missed something, or does an equivalent update need to be made to it as well?

            Show
            swinbank John Swinbank added a comment - It looks like we've only got a new detected-sources_small.txt.expected and not a new detected-sources.txt.expected , which seems surprising. Have I missed something, or does an equivalent update need to be made to it as well?
            Hide
            rowen Russell Owen added a comment - - edited

            We usually only update the _small version, since it's the one used by the integration test and the file is generated by CI itself.

            I don't actually know how to run the script using the exact configuration that CI uses, so I don't know how to manually generate suitable small or large data files.

            Show
            rowen Russell Owen added a comment - - edited We usually only update the _small version, since it's the one used by the integration test and the file is generated by CI itself. I don't actually know how to run the script using the exact configuration that CI uses, so I don't know how to manually generate suitable small or large data files.
            Hide
            swinbank John Swinbank added a comment -

            We usually only update the _small version, since it's the one used by the integration test and the file is generated by CI itself.

            Looking at the log of the files in git, I'm not actually sure that's true. Even if it were, I think it's a problem: we suggest that new users use the full version of the file, so it had better be correct to avoid spreading confusion.

            I don't actually know how to run the script using the exact configuration that CI uses, so I don't know how to manually generate suitable small or large data files.

            Should just be a matter of running the bin/demo.sh script but omitting the --small flag. Note that:

            • This should not depend on configuration; this is what we expect end users to be validating their stack against (as above), so it can't be allowed to vary depending on how they happen to have set things up.
            • You can't just transplant the copy of _small.txt generated by Buildbot, as it is not guaranteed to cause the tests to pass on a Mac (and that's what we'd expect many end users to be using). You'll need to generate on both platforms and check that they're consistent.
            Show
            swinbank John Swinbank added a comment - We usually only update the _small version, since it's the one used by the integration test and the file is generated by CI itself. Looking at the log of the files in git, I'm not actually sure that's true. Even if it were, I think it's a problem: we suggest that new users use the full version of the file, so it had better be correct to avoid spreading confusion. I don't actually know how to run the script using the exact configuration that CI uses, so I don't know how to manually generate suitable small or large data files. Should just be a matter of running the bin/demo.sh script but omitting the --small flag. Note that: This should not depend on configuration; this is what we expect end users to be validating their stack against (as above), so it can't be allowed to vary depending on how they happen to have set things up. You can't just transplant the copy of _small.txt generated by Buildbot, as it is not guaranteed to cause the tests to pass on a Mac (and that's what we'd expect many end users to be using). You'll need to generate on both platforms and check that they're consistent.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Jim Bosch
                Watchers:
                Jim Bosch, John Swinbank, Joshua Hoblitt, Kian-Tat Lim, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: