-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: feedback
Are you sure you want to change the base?
Feedback #1
Conversation
fix: Align modularized code with the original baseline
docs: Add template files
…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
…d integrate unsloth
refactor/GENNLP-29-Unsloth
…-generationfornlp-nlp-05-lv3 into feat/GENNLP-25-wikipedia
…-generationfornlp-nlp-05-lv3 into feat/GENNLP-26-korean-history
…-generationfornlp-nlp-05-lv3 into feat/GENNLP-27-openstax
[FEAT] 외부 데이터 수집 코드 추가
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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 같은 기능으로 데이터를 변환하는 방식을 사용하면 가독성도 높히고 메모리도 아낄 수 있을것 같습니다.
notebooks/ft_data_processing.ipynb
Outdated
There was a problem hiding this comment.
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에 함께 있을 수 있지 않을까합니다.
notebooks/eda.ipynb
Outdated
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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(): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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__": |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여러 파일에 걸쳐서 중복되는 코드들이라서 evaluation_metric에 대하여 따로 나눠서 만들어도 좋을 것 같습니다.
add: translation for openstax vectorstore
👋! 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:
For more information about this pull request, read “Leaving assignment feedback in GitHub”.
Subscribed: @jin-jae @hskhyl @ssunbear @ocean010315 @minjijeong98 @wjddms4299 @gwaksital