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.

Sign up for GitHub

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

Jump to bottom

Address comments on RobustCostFunction implementation #213

Merged
merged 11 commits into from
Jun 14, 2022
Merged

Address comments on RobustCostFunction implementation #213

merged 11 commits into from
Jun 14, 2022

Conversation

luisenp
Copy link
Contributor

@luisenp luisenp commented Jun 10, 2022

This addresses some comments made in #148 after merging.

@luisenp luisenp requested review from mhmukadam and fantaosha June 10, 2022 20:33
@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 Jun 10, 2022
luisenp
luisenp commented Jun 10, 2022
View reviewed changes
Copy link
Contributor Author

@luisenp luisenp left a comment

Choose a reason for hiding this comment

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

@mhmukadam Added responses to the original comments on #148 for quick reference.

theseus/core/loss.py Outdated Show resolved Hide resolved
theseus/core/loss.py Outdated Show resolved Hide resolved
theseus/core/loss.py Outdated Show resolved Hide resolved
@luisenp luisenp changed the title Added EPS constants for robust cost and loss. Address comments on RobustCostFunction implementation Jun 10, 2022
fantaosha
fantaosha reviewed Jun 12, 2022
View reviewed changes
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 except for some minor comments about where to put log_radius and raidus.

theseus/core/robust_cost_function.py Outdated Show resolved Hide resolved
@luisenp luisenp force-pushed the lep.robust_cost_changes branch from 4c69af6 to f31aa06 Compare June 13, 2022 14:59
@fantaosha fantaosha self-requested a review June 13, 2022 15:20
fantaosha
fantaosha approved these changes Jun 13, 2022
View reviewed changes
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.

mhmukadam
mhmukadam approved these changes Jun 13, 2022
View reviewed changes
Copy link
Contributor

@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
Copy link
Contributor Author

luisenp commented Jun 14, 2022

@mhmukadam Fixed your last comments. I also added another commit that makes RobustCostFunction.weight a property, which we need to be able to handle correct vectorization of robust costs (I noticed this in the Bundle Adjustment example). Please take a look.

@luisenp luisenp force-pushed the lep.robust_cost_changes branch from 8386c43 to 517df1b Compare June 14, 2022 15:19
@luisenp luisenp force-pushed the lep.robust_cost_changes branch from 517df1b to 3fd1140 Compare June 14, 2022 15:20
mhmukadam
mhmukadam reviewed Jun 14, 2022
View reviewed changes
theseus/core/loss.py Outdated Show resolved Hide resolved
@luisenp luisenp merged commit 1be3ead into main Jun 14, 2022
@luisenp luisenp deleted the lep.robust_cost_changes branch June 14, 2022 17:04
@luisenp luisenp mentioned this pull request Jul 12, 2022
Closed
@mhmukadam mhmukadam linked an issue Jul 12, 2022 that may be closed by this pull request
Refactor robust cost api #151
Closed
suddhu pushed a commit to suddhu/theseus that referenced this pull request Jan 21, 2023
@luisenp
Address comments on RobustCostFunction implementation (facebookresear…
7a9089e 
…ch#213)

* Added EPS constants for robust cost and loss.

* Renamed Loss -> RobustLoss.

* Changed RobustLoss methods so they receive log radius instead of radius.

* Added unit test file for RobustCostFunction. Still WIP.

* Added unit test for RobustCostFunction's weighted jacobians and errors method.

* Removed NotImplementedError from error() and jacobian() and add warning.

* Made RobustCostFunction's cost weight a property that redirects to that of the base cost function.

* Minor fixes.

* Increased tolerance for RobustCostFunction jacobians test.

* Minor import fix.

* Moved loss.py to robust_loss.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhmukadam mhmukadam mhmukadam approved these changes

@fantaosha fantaosha fantaosha approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor robust cost api
4 participants
@luisenp @mhmukadam @fantaosha @facebook-github-bot