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 :)
| ) | ||
|
|
||
|
|
||
| # IMPORT AND PREPROCESS DATA------------------------------------------------------------------------------ |
There was a problem hiding this comment.
There is no need for these kind of headers, ideally remove them or replace them by more descriptive comments.
There was a problem hiding this comment.
Ideally, you could briefly describe what happens here in the pre-processing: That is, what does this benchmark describe, what is the pre-processing doing and why is it necessary.
Also, general question (also to @AdrianSosic and @Scienfitz ): Wouldn't it be sufficient to just have the pre-processed data as a .csv file here?
There was a problem hiding this comment.
The processing steps clarify how the data is derived. The data are from different source, dfMP from Materials Project and dfExp from experiments.
| # sort the data by load | ||
| dfComposition_temp = dfComposition_temp.sort_values(by="load") | ||
| dfComposition_temp = dfComposition_temp.drop_duplicates(subset="load") | ||
| if len(dfComposition_temp) < 5: # continue to the next composition |
There was a problem hiding this comment.
Why do you continue in this case?
|
|
||
| benchmark_config = ConvergenceExperimentSettings( | ||
| batch_size=1, | ||
| n_doe_iterations=20, |
There was a problem hiding this comment.
Can you elaborate on why you chose these values?
| # 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?
Work in progress Integrated Hardness benchmarking task.
To-do: