Codecov Report

Merging #273 into master will decrease coverage by 6.89%. The diff coverage is n/a.

@@            Coverage Diff            @@
##           master     #273     +/-   ##
=========================================
- Coverage   89.13%   82.24%   -6.9%     
=========================================
  Files         116      116             
  Lines        5834     5834             
=========================================
- Hits         5200     4798    -402     
- Misses        634     1036    +402

corochann commented 5 years ago

@mihaimorariu Could you review and let me know your opinion? This is a matter of where to fix.

test_qm9.sh and evaluate_own_dataset.sh receives argument in order epoch gpu but other shell script receives gpu at first.

This makes the below test_example.sh wrong.

https://github.com/pfnet-research/chainer-chemistry/blob/master/examples/test_examples.sh

So

Option 1: fix test_qm9.sh and evaluate_own_dataset.sh to receive argument in order gpu, epoch
Option 2: fix test_examples.sh to pass argument in correct order epoch gpu for test_qm9.sh execution.

I think Option 1 (current PR) may be easier to understand for users. What do you think?

mihaimorariu commented 5 years ago

@corochann I agree that option 1 is better. Thank you for the feedback!

chainer / chainer-chemistry

examples/qm9/test_qm9.sh: swap parameter position to correctly recognize GPU#0 #273

Codecov Report