search

seznam / flexp

BSD 3-Clause "New" or "Revised" License
8 stars 3 forks source link

9 compare experiments #24

Closed tomasprinda closed 5 years ago

tomasprinda commented 5 years ago

Close #9

browser_experiments

Check doc for more info.

alaneckhardt commented 5 years ago

It looks really nice! I have two comments: 1) Is it possible you forgot to include browse_metrics.py? I don't see it anywhere. 2) Wouldn't it be easier for the user, if the comparison looks directly at the table other_metrics.csv from the example? Usually, I store such a table with more models in one table instead of the metrics.csv, where probably only the best model would be stored.

tomasprinda commented 5 years ago
  1. Is it possible you forgot to include browse_metrics.py? I don't see it anywhere.

It was ignored in .gitignore. Pushed correction.

  1. Wouldn't it be easier for the user, if the comparison looks directly at the table other_metrics.csv from the example?

There is a lot of values. nr_models X nr_metrics. We probably don't want to print all of them in the table so we should pick the best model. How should we do it simply and universally? Maybe take the first row. When creating metrics.csv we just would need to sort it so that the best is the first row. Wouldn't it be confusing?

alaneckhardt commented 5 years ago
  1. Is it possible you forgot to include browse_metrics.py? I don't see it anywhere.

It was ignored in .gitignore. Pushed correction.

Thanks!

  1. Wouldn't it be easier for the user, if the comparison looks directly at the table other_metrics.csv from the example?

There is a lot of values. nr_models X nr_metrics. We probably don't want to print all of them in the table so we should pick the best model. How should we do it simply and universally? Maybe take the first row. When creating metrics.csv we just would need to sort it so that the best is the first row. Wouldn't it be confusing?

I wouldn't mind having all the models I've tried, but that's probably just me, wanting all the info in one place :) If it is possible to write your own metrics parser, than it's fine.

I would wait for @ofilip or @tivvit to look at it as well and merge afterwards.

tomasprinda commented 5 years ago

Then you would need to create more rows for one experiment in the list of all experiments. Is it good idea? It would be problem to sort it by clicking on the table.

tomasprinda commented 5 years ago

We can do something about this issue. @ofilip @tivvit @alaneckhardt @shakukse what do you think about that? Options are:

  1. Leave it as it is. metrics.csv is parsed as following

    • first row (header) is skipped
    • first column is used as metric names
    • second column is used as metric values 
      Metric name; value
acc@5;0.902
pred_time/ex [ms];1.73

  2. Change the metrics.csv to following format, other rows would be ignored. In the experiments table in browser there would be one line per experiment.

    acc@5;pred_time/ex [ms]
0.902;1.73

  3. Change the metrics.csv to following format, all rows would be used. In the experiments table in browser there could be more lines per experiment.

    acc@5;pred_time/ex [ms]
0.902;1.73
0.87;2.73
tomasprinda commented 5 years ago

I vote for 2 because I think it is the most intuitive and simplest option. 3. might be also OK.

ofilip commented 5 years ago

We can do something about this issue. @ofilip @tivvit @alaneckhardt @shakukse what do you think about that? Options are:

other_metrics.csv follows even different format right? It is option 3 + method name. It is what I vote for along with metrics_agg=None (I changed default to None meaning multiple lines. Alternatively user might pick min, max - builtin functions in most cases or anything else)

shakukse commented 5 years ago

I vote for 3 , I think it is better to have all results from experiment.. But 2 is also ok

tomasprinda commented 5 years ago

@ofilip

other_metrics.csv follows even different format right? It is option 3 + method name. It is the same as 3. You can put there any number of columns, with any values and it is printed into the browser.

tomasprinda commented 5 years ago

Ok, I will change it to option 3.

@ofilip I think there is a lot of parameters if you want to use parameter for aggregation. Like column, aggregation_type/no aggregation, handle types of values. This can be done by passing own get_metrics_fcn.

I will define get_metrics_fcn for aggregation in examples so it will be easy to use aggregation and it will be more evident what is happening there.

tomasprinda commented 5 years ago

Done as promised. Please check and merge if OK.

alaneckhardt commented 5 years ago

Merging. Thanks Tomas and sorry for such a long delay :(

tomasprinda commented 5 years ago

Thanks, no problem. 😉 Hope it is useful.

st 16. led 2019 15:36 odesílatel Alan Eckhardt notifications@github.com napsal:

Merging. Thanks Tomas and sorry for such a long delay :(

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/seznam/flexp/pull/24#issuecomment-454695588, or mute the thread https://github.com/notifications/unsubscribe-auth/AXFzw-g-gVRommUNeL3N4UNjD6_2kvrjks5vDuSqgaJpZM4Y08pl .