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

Add ability to construct centered boxes

    XMLWordPrintable

    Details

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

      Description

      As discussed on GitHub, it would be useful if Box2I and Box2D had methods for creating boxes centered on a particular (fractional) point.

      Given that the Box* classes cannot afford more constructors, and we still don't have a good way to discover functions, this capability is best implemented as static factory methods:

      Box2I Box2I::makeCenteredBox(Point2D const& center, Box2I::Extent const& size);
      Box2D Box2D::makeCenteredBox(Point2D const& center, Box2D::Extent const& size);
      

      I do not plan to add a Box2I::makeCenteredBox(Point2I, Extent) method: I'm worried that it might lead to confusion with (Point2D, Extent), and as Jim Bosch pointed out it's not clear what the best behavior is for even-sized boxes.

      The desired functionality is already present in the implementation for Exposure::getCutout, so this work is just a matter of factoring the code, writing new unit tests, and being very careful about the distinctions between Box2I and Box2D.

        Attachments

          Issue Links

            Activity

            No builds found.
            krzys Krzysztof Findeisen created issue -
            krzys Krzysztof Findeisen made changes -
            Field Original Value New Value
            Epic Link DM-14447 [ 80385 ]
            krzys Krzysztof Findeisen made changes -
            Risk Score 0
            krzys Krzysztof Findeisen made changes -
            Link This issue relates to DM-14072 [ DM-14072 ]
            krzys Krzysztof Findeisen made changes -
            Description As discussed on [GitHub|https://github.com/lsst/afw/pull/357#discussion_r192899255], it would be useful if {{Box2I}} and {{Box2D}} had methods for creating boxes centered on a particular (fractional) point.

            Given that the {{Box*}} classes cannot afford more constructors, and we still don't have a good way to discover functions, this capability is best implemented as static factory methods:
            {noformat}
            Box2I Box2I::makeCenteredBox(Point2D const& center, Box2I::Extent const& size);
            Box2D Box2D::makeCenteredBox(Point2D const& center, Box2D::Extent const& size);
            {noformat}

            I do not plan to add a {{Box2I::makeCenteredBox(Point2I, Extent)}} method: I'm worried that it might lead to confusion with {{(Point2D, Extent}}}}, and as [~jbosch] pointed out it's not clear what the best behavior is for even-sized boxes.

            The desired functionality is already present in the implementation for {{Exposure::getCutout}}, so this work is just a matter of factoring the code, writing new unit tests, and being very careful about the distinctions between {{Box2I}} and {{Box2D}}.
            As discussed on [GitHub|https://github.com/lsst/afw/pull/357#discussion_r192899255], it would be useful if {{Box2I}} and {{Box2D}} had methods for creating boxes centered on a particular (fractional) point.

            Given that the {{Box*}} classes cannot afford more constructors, and we still don't have a good way to discover functions, this capability is best implemented as static factory methods:
            {noformat}
            Box2I Box2I::makeCenteredBox(Point2D const& center, Box2I::Extent const& size);
            Box2D Box2D::makeCenteredBox(Point2D const& center, Box2D::Extent const& size);
            {noformat}

            I do not plan to add a {{Box2I::makeCenteredBox(Point2I, Extent)}} method: I'm worried that it might lead to confusion with {{(Point2D, Extent)}}, and as [~jbosch] pointed out it's not clear what the best behavior is for even-sized boxes.

            The desired functionality is already present in the implementation for {{Exposure::getCutout}}, so this work is just a matter of factoring the code, writing new unit tests, and being very careful about the distinctions between {{Box2I}} and {{Box2D}}.
            krzys Krzysztof Findeisen made changes -
            Rank Ranked higher
            krzys Krzysztof Findeisen made changes -
            Rank Ranked lower
            swinbank John Swinbank made changes -
            Sprint AP F18-1 [ 746 ]
            swinbank John Swinbank made changes -
            Rank Ranked lower
            krzys Krzysztof Findeisen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            krzys Krzysztof Findeisen added a comment -

            Hi Jim Bosch, could you review these changes and let me know if everything fits normal Box usage? Thanks!

            Show
            krzys Krzysztof Findeisen added a comment - Hi Jim Bosch , could you review these changes and let me know if everything fits normal Box usage? Thanks!
            krzys Krzysztof Findeisen made changes -
            Reviewers Jim Bosch [ jbosch ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            jbosch Jim Bosch added a comment -

            Looks good - thanks for fixing the documentation on the existing constructors.

            I think it might be best to drop the invert arg or change its defaults - more on that in the PRs.

             

            Show
            jbosch Jim Bosch added a comment - Looks good - thanks for fixing the documentation on the existing constructors. I think it might be best to drop the invert arg or change its defaults - more on that in the PRs.  
            jbosch Jim Bosch made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue relates to RFC-324 [ RFC-324 ]
            Hide
            krzys Krzysztof Findeisen added a comment -

            I'll wait for the re-adoption of RFC-324, and make the interface for makeCenteredBox match whatever is agreed on for the constructors.

            Show
            krzys Krzysztof Findeisen added a comment - I'll wait for the re-adoption of RFC-324 , and make the interface for makeCenteredBox match whatever is agreed on for the constructors.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Per the resolution of RFC-324, makeCenteredBox will not have an invert parameter, and will silently return the empty box when passed negative dimensions.

            Show
            krzys Krzysztof Findeisen added a comment - Per the resolution of RFC-324 , makeCenteredBox will not have an invert parameter, and will silently return the empty box when passed negative dimensions.
            krzys Krzysztof Findeisen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              Assignee:
              krzys Krzysztof Findeisen
              Reporter:
              krzys Krzysztof Findeisen
              Reviewers:
              Jim Bosch
              Watchers:
              Jim Bosch, Krzysztof Findeisen, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.