Generate process graph

[kfu02]

Generating a process graph like those found in GTSFM Figures, but directly from the code. This PR outputs the graph to a .svg file which is then loaded in as a static image to the web interface. A future PR will make the SVG more interactive, potentially with d3.js.

Design

• I used the registry design pattern to define an abstract GTSFMProcess class.
• Any class that serves as a base abstract class should subclass GTSFMProcess. This forces them to implement get_ui_metadata(), which defines the display name, parent plate, and GTSFMProduct names that connect (all as strings) in a dataclass named UiMetadata.
• All children of GTSFMProcess are saved to a central registry when they are defined thanks to Python metaclass magic.
• ProcessGraphGenerator then iterates over this central registry to create a directional graph using pydot, and saves the graph as an SVG file inside rtf_vis_tool/.
• This SVG file is then read and displayed in JS.

I chose to define the graph via the Processes because those are the graph components that are guaranteed to have a uniquely associated file in the codebase. The Products are hardcoded strings because it would be difficult to have the types of Products automatically inferred from code. A future PR could perhaps make the graph instead be parent-plate driven, where each file that defines a parent plate has a list of the Processes it holds.

The current usage is explained at the top of gtsfm_process.py:

# When creating a new class, follow this template:

# 1: Import GTSFMProcess, UiMetadata
# ----------------------------------
# from gtsfm.ui.gtsfm_process import GTSFMProcess, UiMetadata

# 2: Subclass GTSFMProcess (this can replace ABCMeta, since it inherits from ABCMeta)
# ------------------------
# class ClassName(GTSFMProcess):

# 3: Implement get_ui_metadata()
# ------------------------------
#    def get_ui_metadata() -> UiMetadata:
#        """Returns data needed to display node and edge info for this process in the process graph."""
#
#        return UiMetadata(
#                   display_name="Display Name"
#                   parent_plate="Parent Plate Name"
#                   input_products=("Input Product 1", "Input Product 2")
#                   output_products=("Output Product 1", "Output Product 2")
#               )

[akshay-krishnan]

Its a great start, but I have some high-level questions/comments first:

• Do you plan to create another GreyNode class in another PR? I think this will be necessary. The blue nodes should not register string names but actual GreyNode objects. This will avoid the bi-directional edges you currently have. For example the "relative Ts" input to View Graph estimator are different from the relative Ts output by it.

• I think we should call BlueNode and GreyNode something else. Maybe GtsfmProcessor, GtsfmProduct? I also remembered what the pink nodes we discussed were - they were metrics.

• could you explain the solution at a high level? I thought the base classes (for example VerifierBase) would themselves inherit from BlueNode, but that doesn't seem to be the case.

[kfu02]

@akshay-krishnan I edited the PR description to include a high-level description. Sorry, I intended to before requesting reviews but it must've slipped my mind.

To respond to your questions:

1. The issue is that pydot, the graph generator tool I use, only cares about what string literals you give it to display. So a GrayNode class would not solve the underlying issue that the "relative Ts" that go in/out of View Graph Estimator have the same displayed name. A more descriptive name would fix that.

Also, only defining the names of GrayNodes as strings inside BlueNodes reduces the overhead for developers--rather than defining N GrayNode classes and referencing them inside the BlueNode class, one only has to define 1 BlueNode with a list of strings (implied GrayNodes) that connect to it. Since these GrayNodes would just be holding a string display_name I don't see the point in introducing the extra overhead.

2. I agree, but I wasn't sure what to change the name to. I was planning to ask in the meeting on Weds. GTSFMProcessor/Product/Metric makes sense to me. The ROS counterparts would be BlueNode = node, GrayNode = topic, as another option.

3. I originally tried to make the base cases inherit the BlueNode class. The issue is that when iterating over the central registry, I have to call init() on each class to get the useful UI metadata out of each, and often it makes the most sense to define the UI metadata in the abstract superclass (since all concrete implementations should have the same data in and out). So instead I chose the slightly weird option of creating a tiny UI-only class at the tops of files.

I considered making the UI metadata class vars, but then there is no way to enforce that they are set via abstractmethod decorator. But that's another option. I couldn't think of an option that combines the best of both. What do you think?

[dellaert]

Nice ! what's left, or is this ready to merge?

[kfu02]

@dellaert not ready yet, but GH doesn't allow me to un-request reviews. I'm in the process of adding more to the graph

[akshay-krishnan]

Looks quite nice. Can you merge in master? Was also wondering if we can prevent ‘image_pair_indices’ from getting orphaned in the visualization?

By the way, we now support methods that generate matches directly from images (without descriptors). So technically there are two possible graphs now, or two separate pathways, because of the CorrespondenceGenerator. @ayushbaid @akshaykrishnan @dellaert any thoughts about how to visualize that?

It will need more work on Kevin's end to update the graph, once he merges from master :/

[dellaert]

@kfu02 need to merge in master and resolve conflicts as well.

[kfu02]

Looks quite nice. Can you merge in master? Was also wondering if we can prevent ‘image_pair_indices’ from getting orphaned in the visualization?
By the way, we now support methods that generate matches directly from images (without descriptors). So technically there are two possible graphs now, or two separate pathways, because of the CorrespondenceGenerator. @ayushbaid @akshaykrishnan @dellaert any thoughts about how to visualize that?

It will need more work on Kevin's end to update the graph, once he merges from master :/

I'm not sure how to visualize the two separate potential pathways, but I've created an issue for it (#550 ).

[kfu02]

@dellaert I fixed the issue where output products are not in the same plate as their parent process (see PR desc above), it's ready for re-review now

[dellaert]

@dellaert I fixed the issue where output products are not in the same plate as their parent process (see PR desc above), it's ready for re-review now

Super-cool. Quick review based on SVG for now:

• Could we put loader and retriever in a plat as well, see how that looks? It's like a grouping plate, as with bundle adjustment, to get rid of messiness.
• where do "Triangulated Points" come from? Some blue box I presume?
• maybe rename "Multi-View Optimizer" to "Sparse Reconstruction" and add a "Dense Reconstruction" plate for the final step.

[ayushbaid]

Do we detect cases where we have a typo and the input products of one Process does not match the output product of other process?

[kfu02]

No, though that will be obvious in the graph. In the future, people will likely only be adding a few nodes at a time, so I don't think typos will be a large issue.

[kfu02]

• Could we put loader and retriever in a plat as well, see how that looks? It's like a grouping plate, as with bundle adjustment, to get rid of messiness.
• where do "Triangulated Points" come from? Some blue box I presume?
• maybe rename "Multi-View Optimizer" to "Sparse Reconstruction" and add a "Dense Reconstruction" plate for the final step.

Triangulated Points are from a class method in the TwoViewEstimator plate, which is why the node doesn't have a parent.

I will pair program with Hayk to address the first and third comments so he can get a feel for the code as well.

[dellaert]

Looks great, except for CI failure. After you fix that, let me know and we can merge. Might need to enlist help from someone (not me :-))

[akshay-krishnan]

CI error is on the lettuce dataset which seems to be failing on master too. A problem reading those colmap ground truth files since we added the distortion check.
https://github.com/borglab/gtsfm/actions/runs/2920158751

[stepanyanhayk]

CI error is on the lettuce dataset which seems to be failing on master too. A problem reading those colmap ground truth files since we added the distortion check. https://github.com/borglab/gtsfm/actions/runs/2920158751

does this mean I can merge to master now since the failure is not because of my changes?

[akshay-krishnan]

Yes, it looks good to me. Not sure about other reviewers.

[dellaert]

I will merge it :-)
But, @gchenfc could you possibly PR a change to take lettuce out of CI until this is fixed?

[dellaert]

Thanks @kfu02 and @stepanyanhayk !!!

[johnwlambert]

Looks good, thanks all.

@kfu02 looks like thirdparty/d2net/weights/d2_tf.pth got checked in by accident (30 MB). That file should be downloaded by the weight download script. Could you remove it in a follow up PR, and purge it from the git history? Thanks!

[akshay-krishnan]

@johnwlambert I created a PR for that: #557

[kfu02]

@akshay-krishnan it might be good to also add this weights file to the .gitignore in your PR if it is never supposed to be checked in

[akshay-krishnan]

@kfu02 thats true, but I am not sure if we want a very specific third-party path in our gitignore file - lets discuss on that PR. Im happy to add it if John wants it.