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

[Pipeline] Marigold depth and normals estimation #7847

Merged
merged 111 commits into from
May 27, 2024

Conversation

toshas
Copy link
Contributor

@toshas toshas commented May 2, 2024

What does this PR do?

Fixes #7522.
Related to #6533 (community pipeline).

This PR adds the family of Marigold pipelines for dense regression tasks. The depth estimation pipeline is largely an improved version of the community pipeline. The new normals estimation pipeline adds a new functionality of computing surface normals.

Before submitting

Who can review?

@sayakpaul @yiyixuxu

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this. I did a pass through the depth estimation pipeline implementation and left some comments regarding the overall design, some nits, and other things.

I'd wait for @yiyixuxu to review as well before making any changes.

Copy link
Collaborator

@yiyixuxu yiyixuxu left a comment

Choose a reason for hiding this comment

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

thanks for the PR
i left some feedbacks! I agree with all the @sayakpaul 's comments too but I think we should first address the denosing loop, it is very hard to understand what's actually going on right now

@toshas
Copy link
Contributor Author

toshas commented May 27, 2024

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Thanks for the doc related changes.

My main comments are:

So, let's proceed with this plan?

@toshas
Copy link
Contributor Author

toshas commented May 27, 2024

Hi @sayakpaul , I made a round of changes, please let me know what you think about the new separation of the documentation!

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Let's remove the unrelated changes in the toc and then we can merge this!

@sayakpaul sayakpaul merged commit b3d10d6 into huggingface:main May 27, 2024
15 checks passed
@sayakpaul
Copy link
Member

Thanks a mile for landing this one with such poise and greatness, @toshas!

@toshas
Copy link
Contributor Author

toshas commented May 27, 2024

Thanks, @sayakpaul and @yiyixuxu, for guiding me on this one, It was an amazing experience, and I am very happy with the result!

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.

move MarigoldPipeline to core
4 participants