Hardness benchmark #440
Hardness benchmark #440
Conversation
|
|
|
The hardness benchmark is ready for review and some feedbacks. Currently, the bayesian optimization component and multi-task component are set to two Thank you! |
benchmarks/domains/Hardness.py
Outdated
| dfComposition_temp = dfComposition_temp.sort_values(by="load") | ||
| # if there are any duplicate values for load, drop them | ||
| dfComposition_temp = dfComposition_temp.drop_duplicates(subset="load") | ||
| # if there are less than 5 values, continue to the next composition |
There was a problem hiding this comment.
Too verbose I think, comments like this can be removed which are very self-explanatory. Overall, just too many comments like this
There was a problem hiding this comment.
Quick comment from my side as I also have some stuff regarding comments in my review: I agree with @sgbaird that such individual line comments are not necessary. However, I would appreciate a bit more "high-level" comments like "Filtering composition for which less than 5 hardness values are available", descring what a full block of code is doing.
Note that I only unresolved this comment to make it easier for you to spot this comment here of mine, feel free to immediately un-resolve :)
|
Just FYI: I will give my review here mid of January :) |
There was a problem hiding this comment.
First of all, thanks for the benchmark :) This is a very first and quick review since I think that minor changes from your end will simplify the review process for me quite significantly. Also, note that the way that there was a PR involving the lookup mechanism (#441 ) This might (or might not) have an influence on your benchmark here.
Hence, I would appreciate if you could rebase your example onto main, verify that this benchmark is compatible with the new lookup and include the first batch of comments. Then I'll be more than happy to give it a full and proper review :)
| # create a list of dataframes with n samples from dfLookupTable_source to use as initial data | ||
| lstInitialData_temp = [dfLookupTable_source.sample(n) for _ in range(settings.n_mc_iterations)] | ||
|
|
||
| return simulate_scenarios( |
There was a problem hiding this comment.
Something is weird here: You only ever call this with the latest value of n, which is 30. Why do you then create several different campaigns and lists?
There was a problem hiding this comment.
Thank you for pointing that out. Upon review, I realized I’d like to work with the same campaign but with different initial data sizes. Since the initial_data argument is only used in simulate_scenarios, do you have any suggestions on how I could do this elegantly? E.g. could initial_data be specified in Campaign class?
|
Hello @ritalyu17 just for your information: My work load has shifted quite a bit, and it might take some time for me to properly review here. Just wanted to inform you about this :) |
|
Thanks for the information. No rush. |
There was a problem hiding this comment.
Hi @ritalyu17, I can take care of further integration but would like to ask you for two things before I start with my review:
- Can you please rebase the branch on top of the latest main? That is, we need to build the PR on the latest version of the benchmarking module + I'd like to get rid of all the unnecessary merge commits since your PR pretty much orthogonal to what happens else in the repo
- Can you reformat your files to make them compatible with our code conventions? For that, please have look at any other module of the repo and you'll see what I mean. For example, we should consistently use
snake_casefor variable names andCamelCasefor type definitions.
Please ping me once the changes are incorporated (also in the other PR) and I'll have a look 🙃
|
Hi @ritalyu17, any updates from your end? |
|
Hi Adrian, thank you for following up. I am planning on wrapping this up by
end of this week.
…On Tue, Feb 25, 2025 at 8:47 AM AdrianSosic ***@***.***> wrote:
Hi @ritalyu17 <https://github.com/ritalyu17>, any updates from your end?
—
Reply to this email directly, view it on GitHub
<#440 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVQ5HTYKVSH3TG5GVTQQKCT2RRYAJAVCNFSM6AAAAABS5B4BEWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOBSGAZTMNZVHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
[image: AdrianSosic]*AdrianSosic* left a comment (emdgroup/baybe#440)
<#440 (comment)>
Hi @ritalyu17 <https://github.com/ritalyu17>, any updates from your end?
—
Reply to this email directly, view it on GitHub
<#440 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVQ5HTYKVSH3TG5GVTQQKCT2RRYAJAVCNFSM6AAAAABS5B4BEWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOBSGAZTMNZVHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Work in progress Integrated Hardness benchmarking task.
To-do: