Project

General

Profile

Feature #17634

Consider variants of FindMany and friends that perform lookup by Ptr

Added by Christopher Backhouse almost 2 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Start date:
09/01/2017
Due date:
% Done:

0%

Estimated time:
Scope:
Internal
Experiment:
NOvA
SSI Package:
Duration:

Description

I've often found the index-based interface of Find[One|Many][P] clunky. Often in an algorithm you naturally have access to a Ptr to some object, but it takes plenty of extra bookkeeping to keep track of the index in the original vector it came from.

Having just looked at the helpful comments in Assns.h that make clear that an Assn is basically vector<pair<Ptr<A>, Ptr<B>>> it looks like it should be fairly straightforward to provide a Ptr-based Find interface.

Very rough code sketch:

template<class A, class B> class FindManyP2
{
public:
  FindManyP2(Assns<A, B>& assn)
  {
    for(auto it: assn) fMap[it.first].append(it.second);
  }

  std::vector<Ptr<B>> operator[](Ptr<A> p) {return fMap[p];}

protected:
  std::map<Ptr<A>, std::vector<Ptr<B>>> fMap;
}

Obviously I need to account for the case where A and B are swapped in the Assn compared to the query, where there is associated data D, handle errors, etc, but the code seems like the simplest way of expressing the basic idea.

It also occurs to me that this method could probably fix the ambiguities in case of Assns<A, A> (the Ptr knows which collection it's into). We'd love to be able to use such Assns between objects of the same type. We've had to use clunky workarounds a few times to get around their absence.

History

#1 Updated by Kyle Knoepfel almost 2 years ago

  • Status changed from New to Feedback

We are having difficulty understanding exactly which features you are requesting. Can you give an example of the use of the facility you are proposing to illustrate what you desire?

#2 Updated by Christopher Backhouse almost 2 years ago

I'm requesting a way to look up the product B associated with some product A using a Ptr to A, rather than an index in some vector of @A@s

Currently the Find classes work by index:

void analyze(const art::Event& e)
{
  art::Handle<std::vector<Foo>> foos;
  e.getByLabel("foolabel", foos);
  FindOneP<Bar> fmp(foos, e, "assnlabel");

  for(int i = 0; i < foos->size(); ++i){
    Ptr<Foo> pf(foos, i);
    Process(pf, i, fmp);
  }
}

void Process(Ptr<Foo> f, int idx, FindOneP<Bar>& fmp)
{
  Ptr<Bar> b = fmp.at(idx);
  // Do something with f and b
}

Process() doesn't care that f was in a vector, but I still have to pass an index to be able to find the matching b. Obviously in this example we'd just find b in the main loop, but imagine more layers of indirection, where passing the index around because clumsier.

Compare with:

void analyze(const art::Event& e)
{
  // Doesn't require me to pass foos yet
  FindOneP2<Bar> fmp(e, "assnlabel"); // New class

  art::Handle<std::vector<Foo>> foos;
  e.getByLabel("foolabel", foos);

  for(int i = 0; i < foos->size(); ++i){
    Ptr<Foo> pf(foos, i);
    Process(pf, fmp); // no need to pass index
  }
}

void Process(Ptr<Foo> f, FindOneP2<Bar>& fmp)
{
  Ptr<Bar> b = fmp[f];
  // Do something with f and b
}

#3 Updated by Marc Paterno almost 2 years ago

We have a few solutions for your consideration. The first is how we would recommend using FindOne as it currently exists. Note that we have removed the function Process, and introduced DoSomething, which does something with the Foo and Bar objects.

void analyze2(const art::Event& e)
{
  // access and dereference the (valid) handle in one step
  auto const& foos = *e.getValidHandle<std::vector<Foo>>("foolabel");
  // No reason to create Ptr<T> unless we're going to insert it into an Event;
  // reference-to-const is preferable (pointer-to-const if we must allow
  // null pointers).
  FindOne<Bar> bar_for_foo(foos, e, "assnlabel");

  for (auto whichfoo = 0, sz = foos->size(); whichfoo != sz; ++whichfoo) {
     // prefer references to pointers
     Foo const& thisfoo = (*h)[whichfoo];
     Bar const& thisbar = *(bar_for_foo.at(whichfoo));
     // our algorithm knows about types Foo and Bar only
     DoSomething(thisfoo, thisbar);
  }
}

The second, which we prefer in this case, is direct iteration over the Assns object itself:


void analyze3(const art::Event& e)
{
   auto const& assns = *e.getValidHandle<art::Assns<Foo,Bar>>("assnlabel");
   // We use auto below because we like it.
   // The type of pr will be deduced as
   //    std::pair<art::Ptr<Foo>,art::Ptr<Bar>> const&
   for (auto const& pr : assns) { // loop over all associations
     Foo const& thisfoo = *pr.first;
     Bar const& thisbar = *pr.second;
     DoSomething(thisfoo, thisbar);
   }
}

What if there were many Bars for each Foo? The code above will work, but the user would have to work hard if some processing was needed for each "batch" of Bars associated with a given Foo. Saba Sehrish gave a talk at the LArSoft Coordination meeting on July 18 (https://indico.fnal.gov/getFile.py/access?contribId=2&resId=0&materialId=slides&confId=14904), describing the use of the range-v3 library to do such things. That code is not yet released, but is coming soon, and will be available in art (actually canvas), and not just for LArSoft.

Does one of these use patterns meet your need?

#4 Updated by Kyle Knoepfel over 1 year ago

Unless there is need for further discussion, we propose to close this issue based on lack of feedback.

#5 Updated by Christopher Backhouse over 1 year ago

Before the break I actually did some further work on this. Here is an implementation within the NOvA software:
https://cdcvs.fnal.gov/redmine/projects/novaart/repository/diff/branches/feature_findmanybyptr?utf8=%E2%9C%93&rev=28728&rev_to=28726

I converted parts of one major user of Assns over:
https://cdcvs.fnal.gov/redmine/projects/novaart/repository/diff/branches/feature_findmanybyptr?utf8=%E2%9C%93&rev=28731&rev_to=28728
I think the code (which admittedly is a mess both ways) is a little better now.

I didn't find time to really evangelize this within the collaboration to find where we could make best use of it. I do still think it's a much nicer interface than the current FindMany*. No need to worry about whether the index you're using is the right one, you just pass a Ptr and find the associated objects.

Essentially this is the second pattern Marc gives, but with the "batching" wrapped up in a class rather than open-coded by the user. I'm not sure anyone in NOvA knows you can directly iterate an Assn like this though. We may need to do some educating...

#6 Updated by Kyle Knoepfel over 1 year ago

  • Status changed from Feedback to Closed

Since you already seem to have a path forward with your own utility, I suggest you use that for now. Iterating directly through the Assns product may still be the simplest approach here--and a good way of educating others is to use the direct iteration in your own code. :)



Also available in: Atom PDF