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: add notebook to demonstrate use of ArcFaceLoss #680

Merged
merged 4 commits into from
Feb 28, 2023

Conversation

LMMilliken
Copy link
Contributor

@LMMilliken LMMilliken commented Feb 24, 2023

This pr adds a new notebook to the documentation that uses ArcFaceLoss function.


  • This PR references an open issue
  • I have added a line about this change to CHANGELOG

Copy link
Member

@guenthermi guenthermi left a comment

Choose a reason for hiding this comment

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

Nice to have an example for ArcFace! Added some comments

docs/notebooks/image_to_image_arcface.ipynb Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Show resolved Hide resolved
Copy link
Member

@bwanglzu bwanglzu left a comment

Choose a reason for hiding this comment

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

left a minor comment

CHANGELOG.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
Copy link
Member

@scott-martens scott-martens left a comment

Choose a reason for hiding this comment

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

Only about half way through. Why does the notebook and markdown file say the same thing? It's hard for me to keep track on both.

docs/notebooks/image_to_image_arcface.ipynb Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.ipynb Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.ipynb Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.ipynb Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.ipynb Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.ipynb Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
@LMMilliken
Copy link
Contributor Author

@scott-martens The markdown files are created from the notebooks, notebooks are easier to write and markdown files are easier to read in the docs. you can just review the markdown file

@scott-martens
Copy link
Member

I was already most of the way through with the notebook, so I continued to make my suggestions there.

Copy link
Member

@bwanglzu bwanglzu left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@guenthermi guenthermi left a comment

Choose a reason for hiding this comment

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

Looks good, only added some minor comments

docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
docs/notebooks/image_to_image_arcface.md Outdated Show resolved Hide resolved
@github-actions
Copy link

📝 Docs are deployed on https://ft-docs-stanford-cars--jina-docs.netlify.app 🎉

Copy link
Member

@scott-martens scott-martens left a comment

Choose a reason for hiding this comment

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

LGTM

@LMMilliken LMMilliken merged commit 3a008c3 into main Feb 28, 2023
@LMMilliken LMMilliken deleted the docs-stanford-cars branch February 28, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants