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

Implement SpanSet applyFunctor methods

    XMLWordPrintable

    Details

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

      Description

      Implement methods that apply arbitrary functors to pixels within a SpanSet, as described on RFC-37.

      The only tricky part of this implementation will be the "traits" classes that allow different target objects to interpreted differently. I'd be happy to consult on this; I have a rough idea in my head, but it needs to be fleshed out.

        Attachments

          Issue Links

            Activity

            Hide
            nlust Nate Lust added a comment -

            There is no single ticket that is best to review this work descriptions are spread over many tickets but all the work is on just one branch. This ticket will serve well enough. Thank you for agreeing to do this review. The work is all on the tickets/DM-7170 branch on afw. For your part of the review, please review SpanSets.h, SpanSetsFunctorGetters.h and the corresponding parts of the unit test. Within SpanSets.h you are only responsible for reviewing the applyFunctor(Impl) functions. There are a few class methods which make use of that functionality, and you can decide for yourself if you want to look into those or not. If there are any questions, please send me a message here or on Slack.

            Show
            nlust Nate Lust added a comment - There is no single ticket that is best to review this work descriptions are spread over many tickets but all the work is on just one branch. This ticket will serve well enough. Thank you for agreeing to do this review. The work is all on the tickets/ DM-7170 branch on afw. For your part of the review, please review SpanSets.h, SpanSetsFunctorGetters.h and the corresponding parts of the unit test. Within SpanSets.h you are only responsible for reviewing the applyFunctor(Impl) functions. There are a few class methods which make use of that functionality, and you can decide for yourself if you want to look into those or not. If there are any questions, please send me a message here or on Slack.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Does the branch have a pull request? I can't review without one.

            Show
            krzys Krzysztof Findeisen added a comment - Does the branch have a pull request? I can't review without one.
            Hide
            nlust Nate Lust added a comment -

            Feel free to open one, Bob Armstrong is doing the other part of the review. I don't know if you two want to share one pull request or have two. I think one will work fine, but I will let you two decide

            Show
            nlust Nate Lust added a comment - Feel free to open one, Bob Armstrong is doing the other part of the review. I don't know if you two want to share one pull request or have two. I think one will work fine, but I will let you two decide
            Hide
            krzys Krzysztof Findeisen added a comment -

            Sorry for taking so long to get back to you. I've reviewed the code and left comments on GitHub. The biggest issue is documentation; the class needs to conform to DM's current documentation standards before it can be merged. I'd also like to see some improvements made to both the class code (in particular, I think the current names make the code harder to understand than it otherwise would be) and to unit tests. I don't see any issues with the variadic templates themselves, but I'd like to take another look once the code is clearer.

            BTW, I just found out that if the reviewer opens the pull request, then GitHub won't let them either approve the PR or request revisions.

            Show
            krzys Krzysztof Findeisen added a comment - Sorry for taking so long to get back to you. I've reviewed the code and left comments on GitHub. The biggest issue is documentation; the class needs to conform to DM's current documentation standards before it can be merged. I'd also like to see some improvements made to both the class code (in particular, I think the current names make the code harder to understand than it otherwise would be) and to unit tests. I don't see any issues with the variadic templates themselves, but I'd like to take another look once the code is clearer. BTW, I just found out that if the reviewer opens the pull request, then GitHub won't let them either approve the PR or request revisions.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Krzysztof Findeisen John Swinbank I guess that this is due to the new Github review workflow. Perhaps we should make it policy that the person that creates the ticket also always creates the PR?

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Krzysztof Findeisen John Swinbank I guess that this is due to the new Github review workflow. Perhaps we should make it policy that the person that creates the ticket also always creates the PR?
            Hide
            swinbank John Swinbank added a comment -

            In fact, it already is policy — it just isn't widely followed. I'll post something on clo to remind people.

            Show
            swinbank John Swinbank added a comment - In fact, it already is policy — it just isn't widely followed. I'll post something on clo to remind people.
            Hide
            nlust Nate Lust added a comment -

            Krzysztof Findeisen I think I have addressed most of your concerns, though some points I changed to something in between your comments and what I had. When You get a chance, will you take a look at the updated code? There is a commit for code updates and one for unit test updates. These will of course be squashed down after review.

            Show
            nlust Nate Lust added a comment - Krzysztof Findeisen I think I have addressed most of your concerns, though some points I changed to something in between your comments and what I had. When You get a chance, will you take a look at the updated code? There is a commit for code updates and one for unit test updates. These will of course be squashed down after review.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Looks much better, though I still have some concerns about const-correctness and unit test coverage. Feel free to merge after you've gone through the comments on GitHub.

            Show
            krzys Krzysztof Findeisen added a comment - Looks much better, though I still have some concerns about const-correctness and unit test coverage. Feel free to merge after you've gone through the comments on GitHub.
            Hide
            nlust Nate Lust added a comment -

            merged to master

            Show
            nlust Nate Lust added a comment - merged to master

              People

              Assignee:
              nlust Nate Lust
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Krzysztof Findeisen
              Watchers:
              Jim Bosch, John Swinbank, Krzysztof Findeisen, Nate Lust, Pim Schellart [X] (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.