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

Feedback #1

Open
wants to merge 47 commits into
base: feedback
Choose a base branch
from
Open

Feedback #1

wants to merge 47 commits into from

Conversation

github-classroom[bot]
Copy link

@github-classroom github-classroom bot commented Nov 9, 2024

👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to the default branch since the assignment started. Your teacher can see this too.

Notes for teachers

Use this PR to leave feedback. Here are some tips:

  • Click the Files changed tab to see all of the changes pushed to the default branch since the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.
  • Click the Commits tab to see the commits pushed to the default branch. Click a commit to see specific changes.
  • If you turned on autograding, then click the Checks tab to see the results.
  • This page is an overview. It shows commits, line comments, and general comments. You can leave a general comment below.
    For more information about this pull request, read “Leaving assignment feedback in GitHub”.

Subscribed: @kupulau @jinbong-yeom @ASOTEA @EuiInSeong @MUJJINUNGAE @glasshong

github-classroom bot and others added 30 commits November 9, 2024 08:49
@@ -0,0 +1,22 @@
'''
아래에 있는 list를 보고 yaml 파일 수정 후 train.py 실행
Copy link

Choose a reason for hiding this comment

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

config 부분을 매우 잘 정리해서 실험 한 것 같아보입니다.
체계적으로 실험한 면이 돋보이는데, 이런 config들도 실험 갯수가 많아지면 또 관리하기 어려우니,
주요 config들을 다시 결과와 함께 표로 보기 좋게 정리해서 관리하는 것도 좋습니다.

@@ -0,0 +1,12 @@
## Overview
Copy link

Choose a reason for hiding this comment

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

깃헙 활용 방법에 대한 팀 내 룰이 잘 정해진 것 같습니다!

┃ ┗ 📜.keep
┃ ┗ 📜pull_request_templat.md
┣ 📂EDA
┃ ┗ 📜EDA.ipynb
Copy link

Choose a reason for hiding this comment

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

가급적이면 ipynb 이외에 .py 파일로 만들어서 특정 분석을 수행하면 그 결과들이 그래프라면 .png 등의 파일로, 수치나 표라면 csv 등의 파일로 떨어지도록 구현해보는 것도 좋습니다.

┃ ┗ 📜train.py
┃ ┗ 📜trainer.py
┣ 📜.gitignore
┣ 📜Capitate_crop.ipynb
Copy link

Choose a reason for hiding this comment

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

이 파일은 무슨 용도인가요? 알기가 어렵습니다 ㅠ

@@ -0,0 +1,118 @@
import os
Copy link

Choose a reason for hiding this comment

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

추론에 대한 기본적인 기능들은 잘 구현되어있지만, dice 이외의 다양한 metric으로 결과를 자세히 분석하는 내용이 추가되면 좋겠습니다. eda 코드를 따로 둔 것 처럼 결과를 상세분석하는 코드가 추가되면 좋겠습니다.

@@ -0,0 +1,18 @@
import wandb
Copy link

Choose a reason for hiding this comment

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

다른 디렉토리에도 utils가 있어서, 차라리 이 경로는 wandb 등으로 하거나, 따로 디렉토리를 만들지 않고 쓰는 게 더 구성이 깔끔하지 않았을까 생각되네요!

@@ -0,0 +1,22 @@
# wandb.py
import wandb
Copy link

Choose a reason for hiding this comment

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

따로 존재하는 utils/wandb.py와 차이점이 무엇인가요?

┃ ┗ 📂wandb
┃ ┗ 📜wandb.ipynb
┃ ┗ 📜classwise_ensemble.py
┃ ┗ 📜crop_to_2048.ipynb
Copy link

Choose a reason for hiding this comment

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

전처리 등은 ipynb로 관리하면 후에 유지보수가 매우 어렵습니다. py 파일로 관리하도록 변경해보세요!

┃ ┗ 📜9680.csv
┃ ┗ 📂wandb
┃ ┗ 📜wandb.ipynb
┃ ┗ 📜classwise_ensemble.py
Copy link

Choose a reason for hiding this comment

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

앙상블 파일이 두 개로 나눠지는 것도 구성 면에서는 파일이 너무 많아지는 것 같습니다. 좀 더 가독성이 높게 categorization을 하면 좋을 것 같습니다.

┃ ┗ 📜setting.txt
┃ ┗ 📂utils
┃ ┗ 📂ensemble_input
┃ ┗ 📜9542.csv
Copy link

Choose a reason for hiding this comment

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

이런 파일들은 그 의미를 알기가 너무 어렵습니다. 따로 설명을 추가하거나 파일 명을 바꿔주셔야해요!

wandb api key 변환
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.

7 participants