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 :)
| @@ -0,0 +1,284 @@ | |||
| """ | |||
There was a problem hiding this comment.
This autogenerated header should be deleted and should instead have a short one-line description of what this benchmark is about.
|
|
||
| # IMPORT AND PREPROCESS DATA------------------------------------------------------------------------------ | ||
| strHomeDir = os.getcwd() | ||
| dfMP = pd.read_csv( |
There was a problem hiding this comment.
These variable names are not really expressive, could you give some more detailed names? Abbreviating stuff like "df" for "dataframe" is fine, but it is not clear what "MP" is supposed to mean.
There was a problem hiding this comment.
Also, a lot of your variables have redundant or incorrect names: You call a lot of things "temp", but they do not seem to be temporaray but instead are used later on. In my opinion, such variables should then not be called "_temp". Moreover, you tend to include the type of an object in its name, e.g. in lstParameters_bb. This list could however simply be called `parameters". Hence, please review and adjust all of your variable names as this makes it a bit hard to interpret the code.
| ) | ||
|
|
||
|
|
||
| # 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?
| lstElementCols = dfExp.columns.to_list()[4:] | ||
|
|
||
| # ----- FUTHER CLEAN THE DATA BASED ON THE EDA ----- | ||
| # initialize an empty dataframe to store the integrated hardness values |
There was a problem hiding this comment.
Please capitalize the first word of all comments to be consistent with how we write comments.
| # 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?
|
|
||
| __all__ = ["BENCHMARKS"] | ||
|
|
||
| # python -m benchmarks |
There was a problem hiding this comment.
This line should be removed.
| from benchmarks.domains.Hardness import hardness_benchmark, hardness_transfer_learning_benchmark | ||
|
|
||
| BENCHMARKS: list[Benchmark] = [ | ||
| #synthetic_2C1D_1C_benchmark, |
There was a problem hiding this comment.
Please add the original benchmark and remove the comment on the other benchmark as these things are technically independent.
| print("Hardness benchmark is a maximization task on experimental hardness dataset. ") | ||
| print("The dataset is downselect to 94 composition with more than 5 hardness values. ") | ||
| print("The hardness values are integrated using cubic spline interpolation, and the task is to maximize the integrated hardness. ") | ||
| print("") |
There was a problem hiding this comment.
Rather add a "\n" at the end of a previous print if you want to have a new line.
|
|
||
| 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: