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

Eliminate circular aliases in slot centroid definition

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:
      None

      Description

      Serge Monkewitz has discovered that our schema aliases for even the default configuration of measurement algorithms involve cycles, because the slot centroid algorithm contains a reference to its own flag. Fixing this should just involve an extra check in SafeCentroidExtractor.

        Attachments

          Activity

          Hide
          jbosch Jim Bosch added a comment -

          It turns out we already had code that was supposed to fix this (by resolving one alias explicitly when defining another that depended on it), but it was subverted by the fact that the slot aliases weren't being defined until after that code ran. With that fixed, I think we may need to get rid of the need for multi-level alias resolution entirely, which would be a big simplification. I'll explore that idea on DM-3401.

          Show
          jbosch Jim Bosch added a comment - It turns out we already had code that was supposed to fix this (by resolving one alias explicitly when defining another that depended on it), but it was subverted by the fact that the slot aliases weren't being defined until after that code ran. With that fixed, I think we may need to get rid of the need for multi-level alias resolution entirely, which would be a big simplification. I'll explore that idea on DM-3401 .
          Hide
          jbosch Jim Bosch added a comment -

          John Swinbank, smallish review for you that should hopefully be somewhat familiar: this fixes the recursive aliases you saw on DM-1674, where were due to the plugin algorithms being initialized before the slots; that ordering subverted code that was supposed to resolve some aliases immediately. I've also added code to check for that condition and throw, and that in turn required changes to a bunch of test code that was also initializing them out of order.

          All changes are on meas_base tickets/DM-3400; I'll kick off a CI run now, and there's a chance that will inspire some changes to other packages, but they should be trivial.

          Show
          jbosch Jim Bosch added a comment - John Swinbank , smallish review for you that should hopefully be somewhat familiar: this fixes the recursive aliases you saw on DM-1674 , where were due to the plugin algorithms being initialized before the slots; that ordering subverted code that was supposed to resolve some aliases immediately. I've also added code to check for that condition and throw, and that in turn required changes to a bunch of test code that was also initializing them out of order. All changes are on meas_base tickets/ DM-3400 ; I'll kick off a CI run now, and there's a chance that will inspire some changes to other packages, but they should be trivial.
          Hide
          swinbank John Swinbank added a comment -

          Looks good; minor comments on the PRs on meas_base and meas_algorithms, but they're just about tidiness – nothing really substantive.

          Wearing my test-all-of-the-things hat, I'd probably have added a unit test which demonstrates that the the exceptions you've aded in InputUtilities.cc really do get thrown when things are set up out of order, but I don't think that's essential.

          Show
          swinbank John Swinbank added a comment - Looks good; minor comments on the PRs on meas_base and meas_algorithms , but they're just about tidiness – nothing really substantive. Wearing my test-all-of-the-things hat, I'd probably have added a unit test which demonstrates that the the exceptions you've aded in InputUtilities.cc really do get thrown when things are set up out of order, but I don't think that's essential.
          Hide
          jbosch Jim Bosch added a comment -

          I've added a unit test - none of the code in InputUtilities.[h,cc] was being tested directly, and this seemed like a good opportunity to add that.

          Will merge after one more Jenkin's run; adding the test required another slight change to the test utility code, and I want to make sure that doesn't break anything downstream.

          Show
          jbosch Jim Bosch added a comment - I've added a unit test - none of the code in InputUtilities.[h,cc] was being tested directly, and this seemed like a good opportunity to add that. Will merge after one more Jenkin's run; adding the test required another slight change to the test utility code, and I want to make sure that doesn't break anything downstream.
          Hide
          jbosch Jim Bosch added a comment -

          Merged to master. Final CI run did indeed uncover one more trivial change I needed to make, in ip_diffim.

          Show
          jbosch Jim Bosch added a comment - Merged to master. Final CI run did indeed uncover one more trivial change I needed to make, in ip_diffim.

            People

            • Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              John Swinbank
              Watchers:
              Jim Bosch, John Swinbank
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel