Rework Problem::check into a recursive approach

This is the second part of issue #296 (closed) regarding the rework of the check function. Since the problem::print rework was ready, the changes to print in #296 (closed) have already been merged to devel. This issue now only concerns problem::check.

created merge request !361 (merged) to address this issue

mentioned in merge request !361 (merged)

I have pushed the first iteration of the check rework. Have a look when you can @jsola. There is a piece of functionality that I have modified after discussing with @joanvallve :

If each object is responsible for locally checking that they are consistent and then delegating the task of checking the consistency of the rest of the tree to its children, there are some global properties that are not easy to check. In particular, suppose that P is a pointer to problem and F a pointer to a frame. Previous to the rework there was a check of the following form:

P == F->getProblem()

But now, when the frame F is checking its consistency it does not have access to the pointer P. Thus, this check has been substituted by the following assertion

F->getProblem() == F->getTrajectory()->getProblem()

This is not a final decision, it is just a way to have a first working iteration of this rework and go from here. We could pass P as a parameter to recover the original solution, or do whatever we want, it is up for discussion.

added Agreement required ~209 labels

I see.

I guess one way to go is the following:

• Have each child->check(...) accept the pointer to all its parents, that is
  • Hardware -> check (prb_ptr);
  • Sensor -> check (prb_ptr, hrw_ptr) ;
  • Processor -> check (prb_ptr, hrw_ptr, sen_ptr);
  • Trajectory -> check (prb_ptr);
  • Frame -> check (prb_ptr, trj_ptr) ;
  • Capture -> check (prb_ptr, trj_ptr, frm_ptr);
  • Feature -> check (prb_ptr, trj_ptr, frm_ptr, cap_ptr);
  • Factor -> check (prb_ptr, trj_ptr, frm_ptr, cap_ptr, ftr_ptr);
  • Map -> check (prb_ptr);
  • Landmark -> check (prb_ptr, map_ptr) ;
• Add to this scheme the exceptions, notably
  • Capture also needs to pointer to Sensor:
    • Capture -> check (prb_ptr, trj_ptr, frm_ptr, sen_ptr);

Is this enough? What about the factors?

I think that the problem pointer can be compared with the parent's problem pointer, as @jcasals said. Then, it is checked recursively that the pointer is the same for all nodes. It is simpler and it does the job...

Yes, this is true for the problem pointer, so OK for @jcasals's solution!

What about the other pointers? Are they problematic?

pointers from -- to:

• capture to sensor
• factors to all other nodes
• nodes to factors through constrainedBy()
• CaptureMotion to Processor (I think there's a pointer there?)

Factors-ConstrainedByList pointers are bi-directional, checking from the factor that (for instance) shared_from_this is in capture_other->constrained_by_list and viceversa shouldn't be problematic.

The case of the sensor pointer in a Capture... I don't know what can be checked: non nullptr and correctly installed?

Maybe we should do a list of checks for each class? Here?

OK. The key idea about bidirectional pointers is this:

Bidirectional pointers need to be checked twice. Because A --> B --> A may be OK, and also C --> B --> C, but then you go to B and you may find a pointer to D, and either there is no D, or D does not point to B.

That is, you have for example:

• A <--> B
• C <--> B
• D <-- B

Since you cannot get to B from D, you need to check this broken bidirectional link starting from A, C, but also from B.

• Starting from A:

  A --> B --> A ==> OK

• Starting from C:

  C --> B --> C ==> OK

• Starting from D ==> OK, no pointer to check so it's OK

• Starting from B:

  B --> A --> B ==> OK

  B --> C --> B ==> OK

  B --> !D ==> NOK

  B --> D --> !! => NOK

This must be done:

• for all parent/child (it's easy here, because while parent has a list of child pointers, child-->parent has only one pointer)
• and for all factors / constrained_by pairs (less easy, because there are lists of pointers in both directions).

Then for unidirectional pointers...

Maybe @joanvallve you are right and there's nothing to check!

But at least, is it mandatory that every Capture has a pointer to one Sensor? I think it's not necessary, especially for dummy Captures produced at e.g. setPrior() and similar.

Yes, you are right. With this recursive (decentralized) check, bidirectional pointers check is easy. From a node, iterate over all pointers to other (parents/children/others/constrainedby) and check that:

• it is not nullptr.
• the other points back to this node. In the unidirectional case, I wouldn't ckeck nothing if the current implementation is not doing so.

Seems good then!!

OK to proceed!

removed Agreement required label

@jsola have you modified anything related to the check function in your branch? If not I will proceed to merge and close this issue

Maybe... but not significant. Go ahead.

closed via merge request !361 (merged)