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

Change Objective.error() so it can be vectorized w/o changing vectorized cache #363

Merged
merged 9 commits into from
Nov 23, 2022

Conversation

luisenp
Copy link
Contributor

@luisenp luisenp commented Nov 16, 2022

This will be useful for improving the accept/reject step of LM, and for more complex line search later.

Closes #231.

Note that as a consequence of this, this PR changes PGO optimization numerical results because now Objective.error() returns the non-scaled weighted error for RobustCostFunction, while the previous version was returning the scaled one.

@luisenp luisenp added the bug Something isn't working label Nov 16, 2022
@luisenp luisenp self-assigned this Nov 16, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 16, 2022
Copy link
Member

@mhmukadam mhmukadam left a comment

Choose a reason for hiding this comment

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

Looks great!

@luisenp luisenp force-pushed the lep.vectorize_error_only branch from 70a51df to 2a05cd4 Compare November 16, 2022 18:18
@mhmukadam
Copy link
Member

mhmukadam commented Nov 20, 2022

Do any of the PGO example scripts need updating? Edit: based on offline discussion we do not need any updates.

Copy link
Contributor

@fantaosha fantaosha left a comment

Choose a reason for hiding this comment

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

LGTM. Only minor comments.

theseus/core/objective.py Outdated Show resolved Hide resolved
theseus/core/vectorizer.py Show resolved Hide resolved
theseus/core/vectorizer.py Show resolved Hide resolved
@luisenp luisenp merged commit 776063f into main Nov 23, 2022
@luisenp luisenp deleted the lep.vectorize_error_only branch November 23, 2022 17:32
suddhu pushed a commit to suddhu/theseus that referenced this pull request Jan 21, 2023
…zed cache (facebookresearch#363)

* Rename Objective._get_iterator() to be more descriptive.

* Added an independent vectorized method for Objective.error().

* Added unit tests for new vectorization method.

* Added _get_error_iter handling to Objective.vectorized and .disable_vectorization.

* Fix bug in Objective.vectorized property.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RobustCostFunction weighted error is different when vectorization is on/off.
4 participants