Fix minor stuff #920

varunagrawal · 2021-11-09T23:20:34Z

A bunch of minor fixes I made along the way.

johnwlambert · 2021-11-09T23:44:55Z

gtsam/inference/EliminateableFactorGraph-inst.h

@@ -36,17 +36,17 @@ namespace gtsam {
      // no Ordering is provided.  When removing optional from VariableIndex, create VariableIndex
      // before creating ordering.
      VariableIndex computedVariableIndex(asDerived());
-      return eliminateSequential(function, computedVariableIndex, orderingType);
+      return eliminateSequential(orderingType, function, computedVariableIndex);


thanks for the PR, Varun.

Why change the order of variables here? breaks backwards compatibility perhaps?

It doesn't because this is a function call within another function aka the public API is still the same.

Fan and I discovered that the original function call was just calling this function, so instead of the indirect call, we save on the call stack by making this call directly.

Another way to think of this is that we're just calling another version of an overloaded function, but with the added benefit of reducing the call stack, thus minor gains in efficiency plus easier to read the stack trace during debugging.

Cool, makes sense. Could you add a link to the old wrapper function and the new function we're calling directly? Which file are they in?

This is the original one which is deprecated (though not properly).

gtsam/gtsam/inference/EliminateableFactorGraph.h

Line 301 in 744db32

boost::shared_ptr<BayesNetType> eliminateSequential(

This is replaced by this

gtsam/gtsam/inference/EliminateableFactorGraph.h

Line 117 in 744db32

boost::shared_ptr<BayesNetType> eliminateSequential(

I see. So now we are using overloaded versions with boost "none" defaults for missing args. I wonder if we should make a TODO or note to ** fully ** remove the deprecated one as well

Yup, that's issue #889

We don't do TODOs here, we make issues haha

johnwlambert · 2021-11-09T23:45:30Z

python/gtsam/examples/SFMExample_bal.py


-if __name__ == "__main__":
+
+def main():


nit return type here --

def main() -> None:

Adding a return type declaration for None is questionable, since is it even useful? If the return type is something non-trivial, then it helps with things like linting and autocomplete, but None doesn't help with any of that.

I think it's useful purely for complete static analysis

But does the static analysis report it missing? I have all warning enabled (even for the linter) and I get no warnings.

python/gtsam/examples/SFMExample_bal.py

johnwlambert · 2021-11-09T23:46:48Z

python/gtsam/examples/SFMExample_bal.py


-def run(args):
+
+def run(args: argparse.Namespace):


nit: missing return type

python/gtsam/examples/SFMExample_bal.py

akshay-krishnan

All comments I have are a subset of John's, so feel free to merge once he approves.

python/gtsam/examples/SFMExample_bal.py

johnwlambert · 2021-11-10T18:59:54Z

python/gtsam/examples/SFMExample_bal.py

+DEFAULT_BAL_DATASET = "dubrovnik-3-7-pre"
+
+
+def plot_scene(scene_data: gtsam.SfmData, result: gtsam.Values):


looks good. nit here: missing return type

Sorry, I don't see the utility of adding a None return type specification. Unless there is a compelling reason for this, we are losing the forest for the trees.

For me, it makes it easier to immediately understand the function signature.

If we don't have it, the type annotations are not complete, and we can't rely on mypy or other static analyzers completely.

If it's missing, we should be able to assume default None if the docstring doesn't mention a return type. No return type == None makes sense.

We can infer that, but mypy can't : - )

Mypy doesn't report None violations regardless. I added -> None to the run function and did x = run(parser.parse_args()), then I ran mypy and mypy didn't report any issue here, which is the correct behavior.

This goes back to my original comment, this would be useful for autocomplete. However, we're missing the main point that the wrapper doesn't provide type definitions out of the box, so mypy will fail on a lot of other things related to GTSAM before a missing return type None. Plus, we don't even check for type definitions in the CI, so again, I don't see a compelling reason for this review request.

I added in the type-hints.

johnwlambert · 2021-11-10T19:00:40Z

python/gtsam/examples/SFMExample_bal.py

+from gtsam import (GeneralSFMFactorCal3Bundler,
+                   PriorFactorPinholeCameraCal3Bundler, PriorFactorPoint3)
+from gtsam.symbol_shorthand import C, P
+from gtsam.utils import plot


I would import this as something like

import gtsam.utils.plot as gtsam_plot_utils

or

import gtsam.utils.plot as gtsam_plot

Otherwise I get confused about whether this is matplotlib

Which is why I had the functions imported directly. 🙂
If we are okay with that, I can revert to the function import, since this is the case everywhere in the examples.

I think if it's wrong in the other examples, we can slowly make the correct code go viral, starting from here : - )

Importing functions should not be allowed. But if you prefer calling it plot, feel free to leave this change as you have it here

What's the reason for that? Google's style guide mentions Use import statements for packages and modules only, not for individual classes or functions, so we're clearly not abiding by this. 🙂

Plus, I am of the opinion that these are guidelines and not the rules, so we can not conform to them if it means the code is more readable.

Also, ironically, calling it gtsam_plot_utils is no better than simply doing gtsam.utils.plot.whatever_func.

I think many modern Python libraries like Pytorch abide by this. Otherwise it's hard to immediately tell where to go to look for the implementation of a function (do I ctrl-F in this file, or look in another file)

Interesting that Google style forbids the class import, though.

Pytorch does something way wackier. Plus they have a bunch of people making 200K a year for this stuff, not 22K a year like yours truly. 🙂

If you paid me that much money, I'd be more than happy to accommodate this request. 😂

gtsam/nonlinear/factorTesting.h

johnwlambert

LGTM except I prefer the additional type hints. Rest looks good.

johnwlambert · 2021-11-10T19:17:50Z

gtsam/inference/EliminateableFactorGraph-inst.h

@@ -36,17 +36,17 @@ namespace gtsam {
      // no Ordering is provided.  When removing optional from VariableIndex, create VariableIndex
      // before creating ordering.
      VariableIndex computedVariableIndex(asDerived());
-      return eliminateSequential(function, computedVariableIndex, orderingType);
+      return eliminateSequential(orderingType, function, computedVariableIndex);


I see. So now we are using overloaded versions with boost "none" defaults for missing args. I wonder if we should make a TODO or note to ** fully ** remove the deprecated one as well

Because my time is more valuable than a reviewer's pedanticness

varunagrawal added 5 commits November 9, 2021 14:50

fix warning

7d468e9

reduce call stack

bfb21c2

use safer eigen indexing syntax

9165041

wrap Colamd function

a634a91

format and refactor the SFM BAL example

1bcb447

varunagrawal added the cleanup Help clean up old/obsolete aspects of GTSAM label Nov 9, 2021

varunagrawal requested review from johnwlambert and akshay-krishnan November 9, 2021 23:20

varunagrawal self-assigned this Nov 9, 2021

johnwlambert reviewed Nov 9, 2021

View reviewed changes

varunagrawal requested a review from johnwlambert November 10, 2021 16:32

johnwlambert requested changes Nov 10, 2021

View reviewed changes

akshay-krishnan reviewed Nov 10, 2021

View reviewed changes

python/gtsam/examples/SFMExample_bal.py Outdated Show resolved Hide resolved

address reviewer comments

5c9c60a

varunagrawal requested review from johnwlambert and akshay-krishnan November 10, 2021 18:58

johnwlambert reviewed Nov 10, 2021

View reviewed changes

johnwlambert requested changes Nov 10, 2021

View reviewed changes

johnwlambert reviewed Nov 10, 2021

View reviewed changes

gtsam/nonlinear/factorTesting.h Show resolved Hide resolved

johnwlambert approved these changes Nov 10, 2021

View reviewed changes

add return type definitions

0ccb18b

Because my time is more valuable than a reviewer's pedanticness

varunagrawal merged commit f454bcb into develop Nov 11, 2021

varunagrawal deleted the fix/minor-stuff branch November 11, 2021 03:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix minor stuff #920

Fix minor stuff #920

varunagrawal commented Nov 9, 2021

johnwlambert Nov 9, 2021

varunagrawal Nov 9, 2021

varunagrawal Nov 9, 2021

johnwlambert Nov 10, 2021

varunagrawal Nov 10, 2021

johnwlambert Nov 10, 2021

varunagrawal Nov 10, 2021

varunagrawal Nov 10, 2021

johnwlambert Nov 9, 2021

varunagrawal Nov 10, 2021

johnwlambert Nov 10, 2021

varunagrawal Nov 10, 2021

johnwlambert Nov 9, 2021

akshay-krishnan left a comment

johnwlambert Nov 10, 2021

varunagrawal Nov 10, 2021

johnwlambert Nov 10, 2021

varunagrawal Nov 10, 2021

johnwlambert Nov 10, 2021

varunagrawal Nov 10, 2021

varunagrawal Nov 10, 2021

johnwlambert Nov 10, 2021 •

edited

Loading

varunagrawal Nov 10, 2021

johnwlambert Nov 10, 2021

varunagrawal Nov 10, 2021

varunagrawal Nov 10, 2021

johnwlambert Nov 10, 2021

varunagrawal Nov 10, 2021 •

edited

Loading

johnwlambert left a comment

johnwlambert Nov 10, 2021

		DEFAULT_BAL_DATASET = "dubrovnik-3-7-pre"


		def plot_scene(scene_data: gtsam.SfmData, result: gtsam.Values):

Fix minor stuff #920

Fix minor stuff #920

Conversation

varunagrawal commented Nov 9, 2021

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

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 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

johnwlambert Nov 10, 2021 • edited Loading

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

varunagrawal Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

johnwlambert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnwlambert Nov 10, 2021 •

edited

Loading

varunagrawal Nov 10, 2021 •

edited

Loading