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

Lie group checks should be silent for internal operations #486

Open
luisenp opened this issue Mar 20, 2023 · 5 comments
Open

Lie group checks should be silent for internal operations #486

luisenp opened this issue Mar 20, 2023 · 5 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@luisenp
Copy link
Contributor

luisenp commented Mar 20, 2023

When the outcome of something like g1.compose(g2) needs to be normalized, the current code throws a warning. However, this is overly informative for internal ops, and might result in an excessive/intrusive number of warnings. Better to keep these warnings only for when normalizing a user-given tensor at construction time.

@luisenp luisenp added the good first issue Good for newcomers label May 1, 2023
@jacoblubecki
Copy link

I'd like to take a look at this, but from some cursory code-searching, it isn't immediately clear where the warning is occurring. Could you point me in the right direction and I can throw something together?

@luisenp
Copy link
Contributor Author

luisenp commented Feb 23, 2024

Hi @jacoblubecki. Thanks for the offer to help!

These checks happen here. Which is called in the base Manifold class here.

The first option that comes to mind is to extend the _LieGroupCheckContext class to include something like cls.contexts.silent_normalization, which can then be passed in the call to Manifold._check_tensor(). Then in TheseusLayer.forward() you can disable/enable the silent option on enter/exit, w/o changing what the current value of _LieGroupCheckContext.check_lie_group is.

Happy to answer any questions.

@jacoblubecki
Copy link

Thank you, this is perfect. I'll start taking a look! In the meantime, can you assign the ticket to me?

@luisenp
Copy link
Contributor Author

luisenp commented Feb 25, 2024

Assigned. Thanks!

jacoblubecki added a commit to jacoblubecki/theseus that referenced this issue Nov 9, 2024
 ## Problem

The Manifold class will automatically normalize Tensors to make
them valid for their associated Lie group. A warning is emitted
when this occurs, which can be somewhat noisy and undesirable.

Issue: facebookresearch#486

 ## Solution

Add setting to _LieGroupCheckContext which, when set to True, can
be used in downstream code to suppress various warnings.

 ## Testing

Ensure existing unit tests pass.
@jacoblubecki
Copy link

I created a draft pull request -- the CONTRIBUTING.md instructions say to add myself as the assignee and to assign some specific reviewers, but it looks like I don't have permission to set assignees or reviewers on the draft PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants