-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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
The documentation currently pulls files from the unmerged PRs here: Once these are merged, paths to images will need to be updated again |
There was a problem hiding this 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:
- Let's keep the API docs super lightweight.
- All the additional details provided can be repurposed as a guide like https://huggingface.co/docs/diffusers/main/en/using-diffusers/svd.
So, let's proceed with this plan?
This reverts commit a505165150afd8dab23c474d1a054ea505a56a5f.
Hi @sayakpaul , I made a round of changes, please let me know what you think about the new separation of the documentation! |
There was a problem hiding this 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!
Thanks a mile for landing this one with such poise and greatness, @toshas! |
Thanks, @sayakpaul and @yiyixuxu, for guiding me on this one, It was an amazing experience, and I am very happy with the result! |
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
move MarigoldPipeline to core #7522
Marigold - monocular depth estimation pipeline #6533
Who can review?
@sayakpaul @yiyixuxu