Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DiscreteFactorGraph::optimize to return MPE #1050

Merged
merged 21 commits into from
Jan 22, 2022
Merged

Conversation

dellaert
Copy link
Member

Major Changes

This PR fixes #1048. In particular:

  • DiscreteFactorGraph::optimize now really computes the MPE (maximum probable explanation)
  • added a method DiscreteFactorGraph::maxProduct
  • created two versions of each to take OrderingType or Ordering
  • deprecated DiscreteBayesNet::optimize and DiscreteConditional::solve, as they do nothing really useful
  • removed optimalAssignment in gtsam_unstable/discrete: redundant with optimize. Not deprecated, just gone!

Max-product details:

I introduce two new classes (in one header/cpp):

  • DiscreteLookupTable: inherits from DiscreteConditional to play well with generic elimination, but is supposed to be a lookup table of the maximum values obtained during max-product
  • DiscreteLookupDAG: a DAG of lookup tables. Made this separate from BayesNet as to not confuse.
  • maxProduct is implemented with a custom Eliminate function in DiscreteFactorGraph.cpp

@dellaert dellaert self-assigned this Jan 21, 2022
@dellaert dellaert added bugfix Fixes an issue or bug high-priority Need this done quickly labels Jan 21, 2022
@dellaert dellaert requested a review from varunagrawal January 21, 2022 19:59
@dellaert
Copy link
Member Author

@keevindoherty FYI. If you relied optimize, there might be discrepancies with the actual MPE.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments. I can address them if need be to save time.

@dellaert dellaert requested a review from varunagrawal January 21, 2022 23:16
@dellaert
Copy link
Member Author

@varunagrawal sign off? Do you know what could be up with failing CI?

@varunagrawal
Copy link
Collaborator

This is the same issue that Fan had, it's a deprecation of the setup.py install command for python.
You can cherry pick this commit to fix it 2a023f7

@dellaert
Copy link
Member Author

Thanks. Done. Note it's not approved yet so I can't merge... Is there anything else?

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@varunagrawal
Copy link
Collaborator

I was literally in the middle of the review :)

@dellaert
Copy link
Member Author

Hmmm, after the cherry-pick, 10 runs are failing rather than 2… any ideas?

@dellaert
Copy link
Member Author

@varunagrawal could you please explain more about your fix? Linux complains:

Disabling all use of wheels due to the use of --build-options / --global-options / --install-options

What was the issue you tried to solve? What article described the fix? You said “ deprecation of the setup.py install command for python”. Where was it=t deprecated?

finally, use of pip directly seems wrong. If anything, $PYTHON$ -m pip ?

@dellaert
Copy link
Member Author

Also, why does not affect the entirety of GTSAM? If so, seems worthy of its own PR in GTSAM, with explanation of issue in PR description.

@varunagrawal
Copy link
Collaborator

The Python Packaging Authority has decided to deprecate direct builds via calling setup.py (aka python setup.py install) due to the need to enforce a standard for wheel builds. More details here.

This effect first showed up on MacOS with the following error:

python3 setup.py install --user --prefix=
PACKAGES:  ['gtsam', 'gtsam_unstable', 'gtsam.utils', 'gtsam.examples', 'gtsam_unstable.tests', 'gtsam_unstable.examples']
running install
/usr/local/lib/python3.9/site-packages/setuptools/command/install.py:34: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.
  warnings.warn(
/usr/local/lib/python3.9/site-packages/setuptools/command/easy_install.py:156: EasyInstallDeprecationWarning: easy_install command is deprecated. Use build and pip and other standards-based tools.

I myself use a pip install -e . for all my projects which basically asks pip to install the local directory in editable mode. Editable mode means the install reflects any local changes we make, making iterative development very easy.

We are now seeing the deprecation effects in Ubuntu as well, so yeah this is going to need a bit of refactoring of the CI. This is something that really annoys me about Github Actions is that they run environment upgrades pretty arbitrarily. I imagine this will effect the entirety of GTSAM going forward.

About using pip directly compared to ${PYTHON} -m pip, they are equivalent. The pip binary is a tiny python script that imports the pip module and executes the command, making it easier. I can do the latter if you'd like.

@dellaert
Copy link
Member Author

Awesome description above. Perhaps you can copy/paste some of the comment above in the (rather terse) comment of #1054 ?

About GA: can't we specify whatever exact version we want ?

About pip: just heard that sometimes pip on path is a different version so #PYTHON -m pip ensures you have the pip version paired with the python version.

@varunagrawal
Copy link
Collaborator

We can always do pip$PYTHON_VERSION aka the exact pip version to match the python version as well, but I agree with using python -m pip.

# Conflicts:
#	.github/scripts/python.sh
@dellaert dellaert merged commit b441eea into develop Jan 22, 2022
@dellaert dellaert deleted the feature/betterMPE branch January 22, 2022 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes an issue or bug high-priority Need this done quickly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optimize <> MPE
2 participants