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

Fix Typos in Mapper Notebook #26

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

mathyouf
Copy link

@mathyouf mathyouf commented Apr 8, 2021

Purpose

To increase the clarity of reading the dropdown menu.

Typos Fixed

Jhonny Depp -> Johnny Depp
hilary clinton -> Hillary Clinton

Naming Standardized

Old Style: Beyonce, hillary clinton, trump
New Style: Beyonce Knowles, Hillary Clinton, Donald Trump

Ordering By Category then Alphabetical

Old Style: Alphabetical ('Mark Zuckerberg' out of place)

['afro', 'angry', 'Beyonce', 'bobcut', 'bowlcut', 'curly hair', 'Hilary Clinton', 'Jhonny Depp', 'mohawk', 'purple hair', 'surprised', 'Taylor Swift', 'trump', 'Mark Zuckerberg']

New Style: By Category of Hairstyle, Emotion, then Celebrity Likeness

['afro', 'bobcut', 'bowlcut', 'curly hair', 'mohawk', 'purple hair', 'angry', 'surprised', 'Beyonce Knowles', 'Donald Trump', 'Hillary Clinton', 'Johnny Depp', 'Mark Zuckerberg', 'Taylor Swift']

Potential Issue Mitigated

Changing the actual field id's would cause problems with the lookups of the matching mapper/pretrained/{edit_id}.pt so none of them were changed (including hilary_clinton)

mathyouf added 2 commits April 7, 2021 17:11
Some of the names were mispelled, so I corrected the typos
Categories sorted by Hair, Emotion, then Celebrity, within those categories sorted alphabetically
@mathyouf
Copy link
Author

mathyouf commented Apr 8, 2021

Seeing now that the file changes on Jupyter Notebooks are hard to preview, I can imagine it might be difficult to approve this one. The changes I made were small and only to the naming of the key fields in the meta_data dictionary. If you have an alternative way to make contributions or suggestions for PR's, let me know, i'd love to help make this more accessible.

@orpatashnik
Copy link
Owner

Hi @mathyouf ,

I really thank you for your tips! I downloaded the files and looked at them. Looks good overall 😊
A few comments:

  1. I'm not sure I understood what is supposed to be the difference between mapper_playground.ipynb and mapper_playground-checkpoint.ipynb.
  2. About adding Beyonce's last name - I must admit I didn't know that this is her last name 😂 I didn't use it in the driving text so I think it is better to keep the option in the dropdown as 'Beyonce' only. What do you think?

Thanks,
Or

This checkpoint file was generated while testing the notebook changes, and was committed unecessarily
@mathyouf
Copy link
Author

mathyouf commented Apr 8, 2021

Hi @orpatashnik,

Thanks for the appreciation! I hope I can keep contributing to improved visualization tools for this project, since i'm a GAN researcher myself and want to make it easier to navigate and visualize what's going on.

  1. I'm not sure I understood what is supposed to be the difference between mapper_playground.ipynb and mapper_playground-checkpoint.ipynb.

That was checked-in on accident. I've deleted it now in the latest commit.

  1. About adding Beyonce's last name - I must admit I didn't know that this is her last name 😂 I didn't use it in the driving text so I think it is better to keep the option in the dropdown as 'Beyonce' only. What do you think?

I was on the fence about this, because some of the category id's imply that they were trained on truncations of the celebrities name. Examples: Jhonny Depp->depp, Mark Zuckerberg -> zuckerberg, Taylor Swift -> taylor_swift. From a researcher's perspective, the exact string would be the most clear for exact interpretation. From a layperson's perspective, it would be more confusing. What were your thoughts on that, are the id's matches for what it was trained on?

Thanks for taking the time to review this PR. 🙂

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.

2 participants