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

Make BP closer to jax optimizer #135

Merged
merged 9 commits into from
Apr 8, 2022

Conversation

antoine-dedieu
Copy link
Contributor

@antoine-dedieu antoine-dedieu commented Apr 7, 2022

Resolves #124

We make graph.BP closer to JAX optimizers https://jax.readthedocs.io/en/latest/jax.example_libraries.optimizers.html

Copy link
Contributor

@StannisZhou StannisZhou left a comment

Choose a reason for hiding this comment

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

Made a few comments. Main things to do are:

  1. Bundle 5 functions into a dataclass for better organization/documentation and easier usage.
  2. Do some renaming.
    LGTM once these are addressed

pgmax/fg/graph.py Outdated Show resolved Hide resolved
pgmax/fg/graph.py Outdated Show resolved Hide resolved
pgmax/fg/graph.py Outdated Show resolved Hide resolved
@StannisZhou StannisZhou marked this pull request as ready for review April 7, 2022 03:42
@StannisZhou
Copy link
Contributor

StannisZhou commented Apr 7, 2022

Also if you want the corresponding issue to be automatically linked I think you need to say resolves #124

@StannisZhou StannisZhou linked an issue Apr 7, 2022 that may be closed by this pull request
@antoine-dedieu
Copy link
Contributor Author

@StannisZhou all your comments have been adressed

Copy link
Contributor

@StannisZhou StannisZhou left a comment

Choose a reason for hiding this comment

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

LGTM after addressing minor comments:

  1. Change BPContainer to BeliefPropagation (or maybe PropagationFunctions?)
  2. Shorten all bp_container to just bp
  3. Use dataclass instead of NamedTuple
  4. Update docstrings

pgmax/fg/graph.py Outdated Show resolved Hide resolved
pgmax/fg/graph.py Outdated Show resolved Hide resolved
@@ -1002,7 +1044,14 @@ def get_beliefs(bp_arrays: BPArrays) -> Any:
)
return beliefs

return run_bp, get_bp_state, get_beliefs
bp_container = BPContainer(
init=functools.partial(update, None),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean init can only take keyword arguments? If so mention that in the documentation for init in BeliefPropagation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this restriction does not apply, why do you think it would?

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2022

Codecov Report

Merging #135 (aa0f65f) into master (8a31c9c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #135   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          950       967   +17     
=========================================
+ Hits           950       967   +17     
Impacted Files Coverage Δ
pgmax/fg/graph.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a31c9c...aa0f65f. Read the comment docs.

@antoine-dedieu antoine-dedieu merged commit 58fbe95 into vicariousinc:master Apr 8, 2022
@antoine-dedieu antoine-dedieu deleted the run_bp_changes branch April 8, 2022 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update graph.BP interface to be more intuitive
3 participants