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

Add topological-operation-tree Region class and intersection+union operators

    XMLWordPrintable

Details

    • 4
    • Data Release Production
    • No

    Description

      sphgeom.Region doesn't have intersection and union operators because:

      a) the spherical geometry math to perform those operations directly is really hairy
      b) it doesn't have concrete Region subclasses that could be used to represent the results of cross-class intersections or even single-class unions.

      It'd be nice to have those operations available for the Butler query system, however, and a relative easy solution is to create a new Region subclass that just holds a bunch of other Regions in an intersection-union expression tree, and performs all other operations by delegating to that with the right reductions, e.g. "contains point" -> "all(intersected regions contain point)" and "any(unioned regions contain point)". I've started to actually do something like that in Python in daf_butler on DM-31725, but it'd be much more natural (and possibly a lot faster) to do it in C++ and allow the new class to be a Region itself.

      Attachments

        Issue Links

          Activity

            jbosch Jim Bosch added a comment -

            fritzm, I'm hoping this mostly-C++ review of some new sphgeom functionality sounds marginally interesting, and that the change itself doesn't look objectionable from the Qserv side. If you're not able to review, I'd like to send it someone else on the Qserv team, both because you've probably got more experienced C++ devs than pipelines does and at least as much vested interest.

            PR is here.

            FWIW, I also spent some time on this branch trying to put together a double-dispatch system for simplifying unions and intersections instead of relying exclusively on the lazy-evaluation classes. But everything I tried felt like a series of escalating premature optimizations.

            jbosch Jim Bosch added a comment - fritzm , I'm hoping this mostly-C++ review of some new sphgeom functionality sounds marginally interesting, and that the change itself doesn't look objectionable from the Qserv side. If you're not able to review, I'd like to send it someone else on the Qserv team, both because you've probably got more experienced C++ devs than pipelines does and at least as much vested interest. PR is here . FWIW, I also spent some time on this branch trying to put together a double-dispatch system for simplifying unions and intersections instead of relying exclusively on the lazy-evaluation classes. But everything I tried felt like a series of escalating premature optimizations.

            Okay, happy to take a look!

            fritzm Fritz Mueller added a comment - Okay, happy to take a look!

            Nicely factored commits made this pleasant to review – thank you!

            Seems a nice feature to have.  A couple questions left in comments re. completeness of case handling in relates() methods for the new compound regions (possible I am misunderstanding how relates() is supposed to work?)

            Re. double dispatch optos in C++: honestly, I have yet to personally encounter any cure for this that isn't in practice worse than the disease, so I wasn't bothered at all by your approach here 

            fritzm Fritz Mueller added a comment - Nicely factored commits made this pleasant to review – thank you! Seems a nice feature to have.  A couple questions left in comments re. completeness of case handling in relates() methods for the new compound regions (possible I am misunderstanding how relates() is supposed to work?) Re. double dispatch optos in C++: honestly, I have yet to personally encounter any cure for this that isn't in practice worse than the disease, so I wasn't bothered at all by your approach here 

            People

              jbosch Jim Bosch
              jbosch Jim Bosch
              Fritz Mueller
              Fritz Mueller, Jim Bosch
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.