HRKimLab / RLbench

A simple reinforcement learning benchmark framework (under development)
MIT License
5 stars 1 forks source link

mean_plot 수정 요청 내용 #1

Closed olenmg closed 2 years ago

olenmg commented 2 years ago

** Y_AXIS 항목의 경우 공통적으로 들어가는 항목이라 option쪽으로 뺐습니다. (Y_AXIS -> MAPPER_Y)

Major

1. 기능

기능이 잘 작동이 안되는 것 같아서 확인 부탁드립니다 ㅠㅠ 사용한 명령어는 아래와 같습니다. python mean_plot.py --env LunarLanderContinuous-v2 --agents "[a1s1,a2s1]" --x episode --y t --data-path /home/neurlab-dl1/workspace/sb3-practice

image 결과는 위와 같은데, 기대한 결과대로면 a1s1, a2s1 이렇게 두개만 mean-variance plot이 띄워져야 합니다. 혹시 제가 실행을 잘못했다거나 하는 부분이 있으면 말씀해주세요.

데이터 파일 디렉토리 구조는 README에 써진대로입니다. 혹시 모르니 제가 사용한 데이터 파일 구조 아래 캡처 하나 더 첨부해둘게요 image

2. y축 plot 대상

https://github.com/HRKimLab/RLbench/blob/76994f094261c94a59d0d0a114c50dc75d8386c8/src/mean_plot.py#L35 iloc[:, 2] 로 작성하면 argument와 무관하게 무조건 2번 index variable만 plot하게 될거라서, argument를 통해 새로 정의한 y_idx 변수를 이용해서 plot 대상을 정해주세요. (아마 아실텐데 실수로 누락하신 부분인 것 같습니다)

Minor

기능이나 성능과 무관한 내용들입니다.

1. 함수명

https://github.com/HRKimLab/RLbench/blob/76994f094261c94a59d0d0a114c50dc75d8386c8/src/mean_plot.py#L7 저희가 지금 plot하고 있는게 정확히는 mean-variance plot 이기 때문에 함수 이름을 이에 맞춰 잘 바꿔주시면 좋을 것 같습니다. 지금 당장 제가 생각나는건 plot_mean_var 정도인데 이거보다 더 좋은 이름으로 부탁드립니다 🤔

마찬가지로 파일 이름도 mean_plot에서 함수이름과 같은 이름으로 업데이트 부탁드려요.

2. 변수명

https://github.com/HRKimLab/RLbench/blob/76994f094261c94a59d0d0a114c50dc75d8386c8/src/mean_plot.py#L21

만약 dfs가 사용하는 변수라면 dfs보다는 용도에 맞는 더 좋은 이름으로 업데이트 부탁드려요.

3. 주석

plot_numeric에 있는 것처럼 mean_plot 함수가 어떤 기능을 하는지 간단하게 주석 추가 부탁드립니다. https://github.com/HRKimLab/RLbench/blob/76994f094261c94a59d0d0a114c50dc75d8386c8/src/plot_numeric.py#L10-L13

그리고 그 외에도 주석 필요한 부분 있으면 달아주세요~

4. Imports

https://github.com/HRKimLab/RLbench/blob/76994f094261c94a59d0d0a114c50dc75d8386c8/src/mean_plot.py#L1 import 하는 부분 coding convention이 있어서 이거에 맞춰서 수정해주시면 좋을 것 같습니다.

Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.
Imports should be grouped in the following order:

1. Standard library imports.
2. Related third party imports.
3. Local application/library specific imports.
You should put a blank line between each group of imports.
(https://peps.python.org/pep-0008/)

plot_numeric.py에서 import 한거 참고해주세요. https://github.com/HRKimLab/RLbench/blob/76994f094261c94a59d0d0a114c50dc75d8386c8/src/plot_numeric.py#L1-L7

olenmg commented 2 years ago

(0326 22:54) https://github.com/HRKimLab/RLbench/blob/fbfdf9092afaf46f4cfafb844b5386e520734874/src/mean_plot.py#L29-L34

위 코드를 아래와 같이 최적화할 수 있어요 (df/dfs는 변수명이 겹처서 일단 bundles라는 변수명으로 임시로 바꿔서 씁니다) (dfs -> bundles)

for agent, bundle in zip(agent_list, bundles): 
     for file in file_paths: 
         if agent in file: 
             df = pd.read_csv(file, skiprows=1) 
             bundle.append(df) 
             df_concat = pd.concat(bundle) 

파이썬은 인덱스로 배열 요소에 접근하는게 성능이 떨어지거든요, 참고해주시고 이렇게 업데이트 부탁드립니다!

olenmg commented 2 years ago

(0328)

지금까지 말씀드렸던 것들은 수정된 것 모두 확인했고 잘 동작합니다 👍

추가 수정 사항

https://github.com/HRKimLab/RLbench/blob/72fef6b778693c8960d6ff57549a6af10ba87fec/src/plot_mean_var.py#L27-L40

위 부분은 좀 더 좋은 방향으로 수정할 수 있을 것 같아요. 현재 문제점을 찾아본다면,

  1. 27번째 줄에 선언된 bundles 라는 변수는 없어도 될 것 같습니다. for 문을 for agent in agent_list로만 가져가고, bundles를 반복문 밖에 따로 선언하지 않고, 반복문 안에서만 사용하는 리스트를 선언하여 사용해도 같은 결과를 띄울 수 있습니다. 시도해보시고 안되면 말씀해주세요!
  2. 위에서 34번째 줄~ 의 경우, 중첩 for문의 가장 깊은 쪽에 들어가지 않아도 될 것 같습니다. 이것 때문에 불필요한 concat 연산을 여러 번 반복하고 있어요. (원하는 결과를 띄우기 위해 필요한 concat 연산의 최소 횟수는 agent의 개수와 같습니다) meanstd 계산 역시 마찬가지입니다. 이 부분들 모두 반복문 바깥으로 꺼낼 수 있을 것 같아요. 시도해보시고 말씀해주세요!

위 두 가지 사항도 컴퓨터 resource 사용 및 성능에 직접적인 영향을 줄 수 있는 부분이라 추가 수정이 필요합니다. 물론 현재 단계에서 유의미한 차이를 찾기는 힘들 것 같구요 🤔, 추후 데이터가 훨씬 많아지고 이러면, 그때부터는 차이가 생길 수 있어요. (특히 2번의 경우 데이터가 많아지면 속도 차이가 많이 날 수 있습니다)

그리고 원래 작업하시던 mean_plot.py 파일에 마침 git conflict가 나있어서 이런거 어떻게 고치는지 나중에 직접 보면서 말씀드릴게요. (원래라면 이 파일처럼 더 이상 사용하지 않는 파일의 경우 삭제해주셔도 됩니다)

olenmg commented 2 years ago

resolved