Minimum Feedback Arc Set (MFAS) for 1dsfm #387

akshay-krishnan · 2020-07-09T06:43:55Z

This PR implements a solution to the Minimum Feedback Arc Set (MFAS) problem. We use the solution from Kyle Wilson and Noah Snavely's ECCV14 paper.

This change is

varunagrawal

Please run Google formatting on these files.

varunagrawal · 2020-07-11T20:35:53Z

gtsam/sfm/mfas.h

@@ -0,0 +1,65 @@
+/*
+  This file defines functions used to solve a Minimum feedback arc set (MFAS)


Please update the file brief to the GTRC copyright. Do keep the comment about how this is taken from another source.

varunagrawal · 2020-07-11T20:36:28Z

gtsam/sfm/mfas.h

+ * @param edges reference to vector of KeyPairs
+ * @param weights weights corresponding to edges
+ */
+void flipNegEdges(std::vector<KeyPair> &edges, std::vector<double> &weights);


flipNegativeEdges?

varunagrawal · 2020-07-11T20:37:19Z

gtsam/sfm/mfas.h

+ * @param ordered_positions: map from node to position in the ordering (0
+ * indexed)
+ */
+void mfasRatio(const std::vector<KeyPair> &edges,


Is mfas in the function name necessary if we've defined a mfas namespace?

varunagrawal · 2020-07-11T20:38:43Z

gtsam/sfm/mfas.cpp

+  FastMap<Key, double> win_deg;
+  FastMap<Key, double> wout_deg;
+  FastMap<Key, vector<pair<int, double> > > inbrs;
+  FastMap<Key, vector<pair<int, double> > > onbrs;


Use full names to make code more readable?

akshay-krishnan · 2020-07-11T22:05:16Z

Thank you for reviewing @varunagrawal. I am currently working on changing this implementation, but I will make the changes you suggested too.

…_mfas

dellaert

Hey Akshay, great first cut. A bunch of comments, but main one is this: I will need very strong arguments as to why any method would have to be non-const and you store intermediate results as instance variables. We try to be functional, even in C++ :-)

gtsam/sfm/MFAS.cpp

gtsam/sfm/MFAS.h

ayushbaid · 2020-07-22T04:02:47Z

gtsam/sfm/MFAS.h

+
+// used to represent edges between two nodes in the graph
+using KeyPair = std::pair<Key, Key>;
+using TranslationEdges = std::map<KeyPair, Unit3>;


maybe name it UnitTranslationEdges?

While I do appreciate the name, I used the same that was used in TranslationRecovery.h for consistency. We might not use TranslationEdges now that we are changing the interface to vector of edges, so I'm not changing the name right now.

ayushbaid · 2020-07-22T04:06:35Z

gtsam/sfm/MFAS.h

+   * @brief Construct from the nodes in a graph (points in 3D), edges
+   * that are translation directions in 3D and the direction in
+   * which edges are to be projected.
+   * @param nodes Nodes in the graph


Instead of nodes in a graph, we can add something more descriptive. Maybe something like pair of points in 3D?

Or are we making MFAS a generic class which works on any graph and not specific to SLAM?

I've updated the documentation to explain what these nodes represent when used in a translation averaging context.

gtsam/sfm/MFAS.h

ayushbaid · 2020-07-22T04:18:10Z

gtsam/sfm/MFAS.cpp

+  // in each iteration, one node is appended to the ordered list
+  while (orderedNodes_.size() < nodes_->size()) {
+
+    // finding the node with the max heuristic score


separate this in a function?

It is not used anywhere else, but yes, improves readability to factor that into a different function. I shall make that change in the next iteration if no other changes are suggested.

There is a maxim that no function should have naked for loops, and no function should be bigger than 4 lines.

gtsam/sfm/MFAS.cpp

akshay-krishnan · 2020-07-22T06:08:07Z

Thank you for the comments @dellaert and @ayushbaid.

Prof. Frank, this class would be used on graphs with a large number of nodes (which represent cameras in our case). 1dsfm requires several instances of this class to compute the ordering for several sampled directions. These instances would run in parallel. I used the pointers for relativeTranslations_ and nodes_ to avoid storing/copying these large members for each instance. Yes, constant reference members would also serve the same purpose and I am happy to make that change, but for future reference please let me know why we prefer them.

akshay-krishnan · 2020-07-22T06:28:10Z

I can also make most intermediate results internal to the methods, but its a little tricky for positiveEdgeWeights_. For some context, this is computed (but not returned) in computeOrdering, which is called by computeOutlierWeights. But then, computeOutlierWeights also needs positiveEdgeWeights_. Ultimately, 1dsfm only needs computeOutlierWeights, but I thought computeOrdering might be a nice method to have in the future. So the only way I could compute positiveEdgeWeights_ in computeOrdering and use it in computeOutlierWeights without making in an instance variable is through an optional pointer argument to computeOrdering. Is that okay? Otherwise, I could merge both of them into one large method, which I think is not very clean.

dellaert · 2020-07-22T11:28:31Z

Thank you for the comments @dellaert and @ayushbaid.

Prof. Frank, this class would be used on graphs with a large number of nodes (which represent cameras in our case). 1dsfm requires several instances of this class to compute the ordering for several sampled directions. These instances would run in parallel. I used the pointers for relativeTranslations_ and nodes_ to avoid storing/copying these large members for each instance. Yes, constant reference members would also serve the same purpose and I am happy to make that change, but for future reference please let me know why we prefer them.

No, in that case shared pointer is fine, but then document it :-) although, I’m wondering about the design: why not have one class and run a method in parallel?

dellaert · 2020-07-22T11:33:29Z

I can also make most intermediate results internal to the methods, but its a little tricky for positiveEdgeWeights_. For some context, this is computed (but not returned) in computeOrdering, which is called by computeOutlierWeights. But then, computeOutlierWeights also needs positiveEdgeWeights_. Ultimately, 1dsfm only needs computeOutlierWeights, but I thought computeOrdering might be a nice method to have in the future. So the only way I could compute positiveEdgeWeights_ in computeOrdering and use it in computeOutlierWeights without making in an instance variable is through an optional pointer argument to computeOrdering. Is that okay? Otherwise, I could merge both of them into one large method, which I think is not very clean.

Anything that is computed in the constructor is fine as an instance variable. The functional pattern we use is: only constructor is non-const.

…_mfas

akshay-krishnan · 2020-07-29T05:38:18Z

Prof. @dellaert and @ayushbaid could you please re-review and let me know if more changes are necessary? I think it is better to make the change from the map to vector for representing edges in a separate PR.

dellaert

Hard to review as the code is hard to follow and rather c++03 :-/ maybe one more iteration?

dellaert · 2020-07-29T16:50:54Z

gtsam/sfm/MFAS.cpp

+  // in each iteration, one node is appended to the ordered list
+  while (orderedNodes_.size() < nodes_->size()) {
+
+    // finding the node with the max heuristic score


There is a maxim that no function should have naked for loops, and no function should be bigger than 4 lines.

dellaert · 2020-07-29T16:54:05Z

gtsam/sfm/MFAS.cpp

+  }
+
+  // iterate over all edges
+  for (auto it = edgeWeights_.begin(); it != edgeWeights_.end(); it++) {


https://stackoverflow.com/questions/6963894/how-to-use-range-based-for-loop-with-stdmap

dellaert · 2020-07-29T16:54:42Z

gtsam/sfm/MFAS.cpp

+      }
+    }
+    // find its in_neighbors, adjust their out_weights
+    for (auto it = in_neighbors[choice].begin();


Use c++11 for loops, at least :-)

dellaert · 2020-07-29T16:56:41Z

gtsam/sfm/MFAS.cpp

+    }
+  }
+  return outlier_weights;
+}


Add newline

dellaert · 2020-07-29T17:00:39Z

gtsam/sfm/MFAS.cpp

+  }
+}
+
+std::vector<Key> MFAS::computeOrdering() const {


Maybe don’t go to four lines, but this function is indeed too long and complicated. Also ask yourself which STL algorithms you could be using.

dellaert · 2020-08-17T12:22:38Z

@akshay-krishnan merge in develop before doing another push as we changed CI to GitHub actions...

…_mfas

akshay-krishnan · 2020-09-12T22:35:56Z

@dellaert I have made some changes to improve the modularity and readability. Please re-review and let me know if there are more changes I can make.

dellaert

Awesome!

dellaert · 2020-09-18T15:53:06Z

@varunagrawal please approve and merge or comment on what is missing

varunagrawal

LGTM

akshay-krishnan added 4 commits July 2, 2020 23:35

forking code for mfas from 1dsfm

ab047d6

changing mfas to use gtsam keys

636178f

adding tests

23ed115

tests and documentation

b4e5879

akshay-krishnan requested review from binitshah and dellaert July 9, 2020 06:44

formatting headers and removing debugging code

90e7fb8

akshay-krishnan requested review from dellaert and removed request for dellaert July 9, 2020 06:53

varunagrawal added the feature New proposed feature label Jul 10, 2020

varunagrawal requested changes Jul 11, 2020

View reviewed changes

akshay-krishnan changed the title ~~Minimum Feedback Arc Set (MFAS) for 1dsfm~~ Minimum Feedback Arc Set (MFAS) for 1dsfm [WIP] Jul 11, 2020

varunagrawal modified the milestone: GTSAM 4.1 Jul 13, 2020

dellaert added this to the GTSAM 4.1.1 milestone Jul 14, 2020

akshay-krishnan added 2 commits July 20, 2020 23:32

changing MFAS to oops and refactoring

c1c8b0a

minor comments change

0eec6fb

akshay-krishnan changed the title ~~Minimum Feedback Arc Set (MFAS) for 1dsfm [WIP]~~ Minimum Feedback Arc Set (MFAS) for 1dsfm Jul 21, 2020

akshay-krishnan requested review from varunagrawal and ayushbaid and removed request for binitshah July 21, 2020 14:57

Merge branch 'develop' of github.com:borglab/gtsam into feature/1dsfm…

62ba551

…_mfas

dellaert requested changes Jul 21, 2020

View reviewed changes

ayushbaid reviewed Jul 22, 2020

View reviewed changes

akshay-krishnan added 3 commits July 23, 2020 00:06

constructor API change after review1

e7214a7

Merge branch 'develop' of github.com:borglab/gtsam into feature/1dsfm…

698ec27

…_mfas

changes after review - removing positiveEdgeWeights

b25809d

akshay-krishnan requested review from dellaert and ayushbaid July 29, 2020 05:30

dellaert reviewed Jul 29, 2020

View reviewed changes

Merge branch 'develop' of github.com:borglab/gtsam into feature/1dsfm…

b81365e

…_mfas

akshay-krishnan marked this pull request as draft September 12, 2020 20:30

code changes to increase modularity

43af7c4

akshay-krishnan changed the title ~~Minimum Feedback Arc Set (MFAS) for 1dsfm~~ Minimum Feedback Arc Set (MFAS) for 1dsfm [WIP] Sep 12, 2020

akshay-krishnan and others added 3 commits September 12, 2020 14:22

fixes1

0d41941

fixes2

539ebb0

fixes3

0ab7f4c

akshay-krishnan changed the title ~~Minimum Feedback Arc Set (MFAS) for 1dsfm [WIP]~~ Minimum Feedback Arc Set (MFAS) for 1dsfm Sep 12, 2020

akshay-krishnan marked this pull request as ready for review September 12, 2020 22:33

dellaert requested a review from jingwuOUO September 14, 2020 14:59

akshay-krishnan requested review from dellaert and removed request for varunagrawal September 18, 2020 02:04

dellaert approved these changes Sep 18, 2020

View reviewed changes

dellaert requested review from varunagrawal and removed request for ayushbaid and jingwuOUO September 18, 2020 12:33

jingwuOUO approved these changes Sep 18, 2020

View reviewed changes

varunagrawal approved these changes Sep 18, 2020

View reviewed changes

varunagrawal merged commit 9350dd7 into develop Sep 18, 2020

varunagrawal deleted the feature/1dsfm_mfas branch September 18, 2020 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimum Feedback Arc Set (MFAS) for 1dsfm #387

Minimum Feedback Arc Set (MFAS) for 1dsfm #387

akshay-krishnan commented Jul 9, 2020 •

edited

Loading

varunagrawal left a comment

varunagrawal Jul 11, 2020

varunagrawal Jul 11, 2020

varunagrawal Jul 11, 2020

varunagrawal Jul 11, 2020

akshay-krishnan commented Jul 11, 2020

dellaert left a comment

ayushbaid Jul 22, 2020

akshay-krishnan Jul 25, 2020

ayushbaid Jul 22, 2020

akshay-krishnan Jul 29, 2020

ayushbaid Jul 22, 2020

akshay-krishnan Jul 29, 2020

dellaert Jul 29, 2020

akshay-krishnan commented Jul 22, 2020 •

edited

Loading

akshay-krishnan commented Jul 22, 2020

dellaert commented Jul 22, 2020

dellaert commented Jul 22, 2020

akshay-krishnan commented Jul 29, 2020

dellaert left a comment

dellaert Jul 29, 2020

dellaert Jul 29, 2020

dellaert Jul 29, 2020

dellaert Jul 29, 2020

dellaert Jul 29, 2020

dellaert commented Aug 17, 2020

akshay-krishnan commented Sep 12, 2020

dellaert left a comment

dellaert commented Sep 18, 2020

varunagrawal left a comment

		@@ -0,0 +1,65 @@
		/*
		This file defines functions used to solve a Minimum feedback arc set (MFAS)

Minimum Feedback Arc Set (MFAS) for 1dsfm #387

Minimum Feedback Arc Set (MFAS) for 1dsfm #387

Conversation

akshay-krishnan commented Jul 9, 2020 • edited Loading

varunagrawal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshay-krishnan commented Jul 11, 2020

dellaert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshay-krishnan commented Jul 22, 2020 • edited Loading

akshay-krishnan commented Jul 22, 2020

dellaert commented Jul 22, 2020

dellaert commented Jul 22, 2020

akshay-krishnan commented Jul 29, 2020

dellaert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dellaert commented Aug 17, 2020

akshay-krishnan commented Sep 12, 2020

dellaert left a comment

Choose a reason for hiding this comment

dellaert commented Sep 18, 2020

varunagrawal left a comment

Choose a reason for hiding this comment

akshay-krishnan commented Jul 9, 2020 •

edited

Loading

akshay-krishnan commented Jul 22, 2020 •

edited

Loading