new parallel advantage assessment support #685
Conversation
|
Thanks for the updates! There are only a few comments remaining that still need to be discussed or fixed. Many are related to discussing whether the plotting function should be reusing regret.
|
These 2 selected was already handled. I need find a time slot for other potentials code refines, but this one #685 (comment) should be easy and can be done soon. |
Alright, thanks! |
…allel-assesment
…allel-assesment
src/orion/plotting/backend_plotly.py
Outdated
| duration_col = "duration_mean" if "duration_mean" in exp_data else "duration" | ||
| durations = exp_data[duration_col].tolist() | ||
| unit_time = durations[-1] / len(durations) | ||
| if unit_time > 60 * 60 * 24: |
There was a problem hiding this comment.
Factoring out this part as a function would make it easy to test it separately. I think we should have a min number of units to select seconds, minutes, etc. If we select minute instead of seconds for instance, there should be at least 3 of 4 minutes so that the tick labels are > 1. Otherwise it would hurt readability. The factors (ex: 60 * 60 * 24) could be multiplied by this min number of units.
There was a problem hiding this comment.
You mean factoring the code to compute duration and time_unit as a function?
Let's make the min number of unit as 3.
There was a problem hiding this comment.
Yep, having a function like this duration, unit_time = infer_unit_time(durations, min_unit=3)
There was a problem hiding this comment.
updated in 884eaff
There was a problem hiding this comment.
Looks like the test did not cover this case: 884eaff#diff-3de0162ba5d54eda9f327045182609e93571e11db5ad711067d7c54c02155d52R633. Otherwise it's good! :)
| experiments, | ||
| with_evc_tree=True, | ||
| order_by="suggested", | ||
| build_frame_fn=build_regrets_frame, |
There was a problem hiding this comment.
That's a great way of factoring it out! I think the regrets implementation will be better after this PR than it was before. Thanks! :)
|
Beside code coverage for the x-axis label and unit, and the minimum number of units for the conversion, I believe this PR all good and will be ready for merge. Thanks! |
|
Oh, it looks like there is an issue with the rebase on develop after merge of #810. |
|
Fixed in #828 |
…allel-assesment
Description
Implementation for #657
Changes
Support a new assessment
ParallelAdvantageChecklist
Tests
$ tox -e py38; replace38by your Python version if necessary)Documentation
Quality
$ tox -e lint)