johko / computer-vision-course

This repo is the homebase of a community driven course on Computer Vision with Neural Networks. Feel free to join us on the Hugging Face discord: hf.co/join/discord
MIT License
452 stars 139 forks source link

Update toctree #208

Closed ratan closed 7 months ago

ratan commented 7 months ago
ratan commented 7 months ago

Hi @mmhamdy Thanks for the feedback. I restricted this PR only to toc file now.

ratan commented 7 months ago

@bellabf @mmhamdy thanks for your feedback and latest commit has all your recommendations. I see that many other PR are touching the toc file for few chapters and it will be rebase issue as this PR changes many chapter first time in toc file, making many lines of changes. Can we merge this PR in this form? @bellabf i have reached to chapter owner for correct ordering, but only heard from one who will revert back by next week. Can i fix the reorder issue in chap 4, 5, 8 in next PR?

ratan commented 7 months ago

Thanks @merveenoyan, done the changes as suggested by you. Please have a look

ratan commented 7 months ago

Thats a good suggestion. However the folder naming convention is is different. so toc file can be tuned for either of the way. Lets @merveenoyan @bellabf also share what the final decision and the i will push the commit accordingly.

bellabf commented 7 months ago

Thats a good suggestion. However the folder naming convention is is different. so toc file can be tuned for either of the way. Lets @merveenoyan @bellabf also share what the final decision and the i will push the commit accordingly.

Thank you for all your good work and quick reactivity! @ratan :smile:

Great suggestion, @mmhamdy! I personally do not care too much on the style name the files as long as we follow the same style across all the course. IMO, the problem is that we are not aware of the order of the chapter for all the units (which I am guessing would make changing to the NLP convection harder, and there even be chapters that are self-contained and could fit in multiple places). The pro is that we would be forced to find out the order. The con is that if we get it wrong even once, there would be lots of renaming (which might make tracking the files a bit messy).

I think @merveenoyan has a better grasp on this and its long term impact, so I will wait for her opinion before re-reviewing the PR again.

Thanks again everyone :hugs:

ratan commented 7 months ago

Hi @mmhamdy sure, you are most welcome. I have dropped DM to few authers but didnt get confirmation. If you can help with ordering for 4, 5, 8, it will be really appreciated.

mmhamdy commented 7 months ago

Great! That sounds cool, I'll check the ordering in these units.

johko commented 7 months ago

Thanks for taking care of this :pray:

merveenoyan commented 7 months ago

@ratan we are following the one made for Audio Course: https://huggingface.co/learn/audio-course/chapter0/introduction

ratan commented 7 months ago

@merveenoyan are you suggesting to change the chapter names as suggested inhttps://huggingface.co/learn/audio-course/chapter0/introduction in this repo and update toc file? Hope this doesn't create big rebase issue with other PR already being reviewed? We can divide it into 2 parts?

  1. merge this PR as such with order for 4,5,8 fixed
  2. Later when 1 or 2 PR touching toc files is merged, i can create another PR just to change the chapter names Please suggest @bellabf @mmhamdy
kfahn22 commented 7 months ago

I just wanted to add that I think the READMEs in the Unit folders will need to either be moved (if you want to keep them for posterity) or deleted.

ratan commented 7 months ago

@merveenoyan are you suggesting to change the chapter names as suggested inhttps://huggingface.co/learn/audio-course/chapter0/introduction in this repo and update toc file? Hope this doesn't create big rebase issue with other PR already being reviewed? We can divide it into 2 parts?

  1. merge this PR as such with order for 4,5,8 fixed
  2. Later when 1 or 2 PR touching toc files is merged, i can create another PR just to change the chapter names Please suggest @bellabf @mmhamdy

Please advise we if can go ahead with this suggestion?

ratan commented 7 months ago

@merveenoyan are you suggesting to change the chapter names as suggested inhttps://huggingface.co/learn/audio-course/chapter0/introduction in this repo and update toc file? Hope this doesn't create big rebase issue with other PR already being reviewed? We can divide it into 2 parts?

  1. merge this PR as such with order for 4,5,8 fixed
  2. Later when 1 or 2 PR touching toc files is merged, i can create another PR just to change the chapter names Please suggest @bellabf @mmhamdy

Please advise we if can go ahead with this suggestion?

@bellabf @mmhamdy @merveenoyan needs your input. Can you please check the above comment?

merveenoyan commented 7 months ago

@ratan AFAIK it should've followed already, but we can open a new PR or handle it in this PR if possible

merveenoyan commented 7 months ago

@ratan if you can run styling in this PR we can merge it

ratan commented 7 months ago

merging the changes