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

Docs seam carve #110

Merged
merged 9 commits into from
Jan 24, 2025
Merged

Docs seam carve #110

merged 9 commits into from
Jan 24, 2025

Conversation

Arc-Celt
Copy link
Collaborator

@Arc-Celt Arc-Celt commented Jan 23, 2025

Hi team! I completed my documentation draft for seam carving function. You can find it in docs/seam_carving.ipynb, and the rendered html should look like this:

image

I haven't add scikit-image to poetry yet due to some weird issues. If anyone have time, you can add it to poetry!

Also, any ideas on how we can suppress the warnings in the html output? Because it might not be appropriate to show the my script directory to the users, as you can see below:

image

@Arc-Celt Arc-Celt added the documentation Improvements or additions to documentation label Jan 23, 2025
@Arc-Celt Arc-Celt self-assigned this Jan 23, 2025
@Arc-Celt
Copy link
Collaborator Author

After discussing with Daniel, I'll suppress the warnings and tell the users that they're expected to see a warning, but I suppress it for better display.

Copy link
Collaborator

@JennyonOort JennyonOort left a comment

Choose a reason for hiding this comment

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

Just a comment from my side!

Copy link
Collaborator

@JennyonOort JennyonOort Jan 24, 2025

Choose a reason for hiding this comment

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

I like the flow of the tutorial and it is very clearly laid out. Just one thought (up to you to determine if you would like to make the change), when both horizontal and vertical seam carving are applied, the final picture, when it is a person, lose its originality seemingly a lot, just by nature of the image.

With that being said, would it make sense for the third example (or maybe all examples) to be a scenary image of some sort. That is will be less visually obvious to the human eye in terms of carving and a more elegant demonstration of why seam-carving is useful in real world setting. The current result on a human photo would likely make people less inclined to try.

I've looked into other possible images available in libraries. And turns out sklearn has its own RGB images for demonstration, which is also what 512 lab used. LOL

Here is the link to documentation on how to load the images if you decide to change.
https://scikit-learn.org/1.6/modules/generated/sklearn.datasets.load_sample_images.html

One caveat with this library is that it only have two RGB images, so would not work for my tasks. But I think it is okay if we have two libraries from which we extract sample image. skimage also has a space launching color image. Maybe that is for consideration too!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the advice! I have changed the astronaut portrait photo to the rocket launching one from skimage.

@Arc-Celt Arc-Celt requested a review from JennyonOort January 24, 2025 18:30
Copy link
Collaborator

@JennyonOort JennyonOort 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! Clear and clean demo that is visually appealing as well.

@Arc-Celt Arc-Celt merged commit 33d8f90 into main Jan 24, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants