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 96 commits into
base: feedback
Choose a base branch
from
Open

Feedback #1

wants to merge 96 commits into from

Conversation

github-classroom[bot]
Copy link

@github-classroom github-classroom bot commented Nov 8, 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: @jin-jae @hskhyl @ssunbear @ocean010315 @minjijeong98 @wjddms4299 @gwaksital

github-classroom bot and others added 30 commits November 8, 2024 09:40
fix: Align modularized code with the original baseline
…ibution

feat: Add function to create uniform answer distribution
merge: Baseline reproduction done on develop branch
…-Analysis

GENNLP-9: feat: Add EDA script for data analysis
ssunbear and others added 24 commits December 2, 2024 16:54
[FEAT] 외부 데이터 수집 코드 추가
Copy link

Choose a reason for hiding this comment

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

데모 데이터를 전처리하기 위한 코드로 main readme 파일에 설명을 넣어주셨지만, 해당 부분에 대하여 notebook으로 보다는 python 파일로 만들어서 demo 쪽으로 통합을 해도 괜찮지 않을까합니다.
노트북을 쓰는 이유가 개인적으로 시각화 목적이 1순위라고 생각해서 시각화가 없다면 일반 python으로 작성하는 것이 활용성면에서 더 좋지 않을가 합니다.

"transformed_data = []\n",
"\n",
"# DataFrame의 각 행을 순회하며 데이터 변환 수행\n",
"for idx, row in df.iterrows():\n",
Copy link

Choose a reason for hiding this comment

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

iterrow를 통해서 한줄씩 dataframe을 읽고 새로운 dataframe을 만듬으로써 id나 중복 데이터를 만드는 방식의 경우 큰 데이터에서는 중복된 데이터로 많은 메모리를 쓰게 되어서 map, apply 같은 기능으로 데이터를 변환하는 방식을 사용하면 가독성도 높히고 메모리도 아낄 수 있을것 같습니다.

Copy link

Choose a reason for hiding this comment

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

파일 이름이 ft_data_processing이여서 직관적이지 않은 것 같습니다.
또한 이름과 함께 주피터 노트북이라서 필수로 실행해야하지 않아도 되는 파일로 느껴져서 해당 부분의 코드를 일반 파이썬 파일로 하면서 프로젝트에서 src/preprocessing.py에 함께 있을 수 있지 않을까합니다.

Copy link

Choose a reason for hiding this comment

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

시각화 데이터를 잘 만들어 주셨는데, 노트북을 웹에서 업로드 하는 이유가 데이터 시각화와 함께 글을 남기는 거라고 생각을 해서 각 그래프에 대한 설명을 아래에 Markdown으로 내용을 정리해서 업로드를 하면 분석 결과를 해당 파일만으로도 파악이 더 잘되지 않을까합니다.

records.append(record)

# Convert processed records to a DataFrame
data_df = pd.DataFrame(records)
Copy link

Choose a reason for hiding this comment

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

apply를 이용하는 것이 가독성이나 재사용성에서 좋을 것 같습니다.

print("\n정답 개수 분포:")
print(final_df['answer'].value_counts().sort_index())

def main():
Copy link

Choose a reason for hiding this comment

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

csv_files랑 weights에 대하여 하드 코딩 된 부분을 main function 밖으로 빼거나 args를 받을 수 있으면 좋을 것 같아요

src/model.py Outdated
Copy link

Choose a reason for hiding this comment

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

구조는 선호에 따라다르다고 생각하는데, 현재 model.py 안에 model train, tokenize, evaluation 기능등이 다 들어 있어서 trainer 같은데요. 해당 클래스 내 너무 많은 기능이 있어서 분리하는 것이 필요할 수 있을 듯합니다.



# 사용 예시
if __name__ == "__main__":
Copy link

Choose a reason for hiding this comment

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

해당 코드가 외부 데이터를 가지고 와서 전처리를 하는 파이프라인 코드로 보이는데요.
기존 모델 학습 코드랑 한 폴더에 있어서 분리를 하고 해당 파일도 전처리 뿐만 아니라 크롤링 부터 해서 다양한 역할이 한 파일에 다 들어가 있어서 분리를 하는 것도 좋을 것 같습니다.

# List of retrieved documents
references = eval(row['reference'])

K = len(references) # Number of retrieved documents (Top K)
Copy link

Choose a reason for hiding this comment

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

각 방법론 마다 metric 계산하는 코드의 중복이 있는 듯 합니다. 함수로 묶어서 처리를 하면 좋을 것 같습니다.

return []


def evaluate_metrics_threshold(df, retriever):
Copy link

Choose a reason for hiding this comment

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

여러 파일에 걸쳐서 중복되는 코드들이라서 evaluation_metric에 대하여 따로 나눠서 만들어도 좋을 것 같습니다.

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