Hardness benchmark #440

ritalyu17 · 2024-12-03T06:48:20Z

Work in progress Integrated Hardness benchmarking task.

To-do:

replace the dataset

CLAassistant · 2024-12-03T06:48:26Z

All committers have signed the CLA.

benchmarks/domains/Hardness.py

ritalyu17 · 2024-12-16T08:17:35Z

The hardness benchmark is ready for review and some feedbacks.

Currently, the bayesian optimization component and multi-task component are set to two Benchmark. Main reason for seperating them is because the arguments in simulate_scenarios are different, specifically initial_data. Maybe there is a way to make the code look nicer?

Thank you!

sgbaird · 2024-12-16T18:49:47Z

benchmarks/domains/Hardness.py

+    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


Too verbose I think, comments like this can be removed which are very self-explanatory. Overall, just too many comments like this

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 :)

benchmarks/domains/Hardness.py

AVHopp · 2024-12-19T16:41:09Z

Just FYI: I will give my review here mid of January :)

AVHopp

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 :)

benchmarks/domains/Hardness.py

AVHopp · 2025-01-08T14:20:02Z

benchmarks/domains/Hardness.py

+)
+
+
+# IMPORT AND PREPROCESS DATA------------------------------------------------------------------------------


There is no need for these kind of headers, ideally remove them or replace them by more descriptive comments.

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?

The processing steps clarify how the data is derived. The data are from different source, dfMP from Materials Project and dfExp from experiments.

benchmarks/domains/Hardness.py

AVHopp · 2025-01-08T14:30:09Z

benchmarks/domains/Hardness.py

+    # 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


Why do you continue in this case?

Given that hardness without load is not meaningful, we are using integrated hardness here. A cubic spline interpolation is used to make the hardness vs. load curve. Compositions with fewer than 5 data points are excluded from the analysis to prevent errors during interpolation.

benchmarks/domains/__init__.py

benchmarks/domains/Hardness.py

AVHopp · 2025-01-08T14:34:25Z

benchmarks/domains/Hardness.py

+
+benchmark_config = ConvergenceExperimentSettings(
+    batch_size=1,
+    n_doe_iterations=20,


Can you elaborate on why you chose these values?

Values are chosen based on the behavior observed in the convergence curve. After 20 iterations with batch size at 1, the optimization curve shows minimal change in the results and further iterations would not significantly improve the outcome.

For real-world application, one may want to have larger batch size and smaller iteration for quick turn around time.

AVHopp · 2025-01-08T14:36:31Z

benchmarks/domains/Hardness.py

+        # 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(


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?

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?

AVHopp · 2025-01-28T11:54:25Z

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 :)

ritalyu17 · 2025-01-28T14:23:36Z

Thanks for the information. No rush.

AdrianSosic

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_case for variable names and CamelCase for type definitions.

Please ping me once the changes are incorporated (also in the other PR) and I'll have a look 🙃

AdrianSosic · 2025-02-25T13:47:22Z

Hi @ritalyu17, any updates from your end?

ritalyu17 · 2025-02-25T21:11:44Z

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: ***@***.***>

Hardness benchmark

54bd0f5

ritalyu17 added 2 commits December 3, 2024 09:24

Upload dataset

1c46668

Delete benchmarks/domains/exp_hardness.csv

b2dd010

AVHopp reviewed Dec 9, 2024

View reviewed changes

benchmarks/domains/Hardness.py Outdated Show resolved Hide resolved

ritalyu17 added 4 commits December 16, 2024 03:04

Merge branch 'emdgroup:main' into Hardness

1208c29

Delete benchmarks/domains/Hardness.py

9b95e32

Delete benchmarks/domains/__init__.py

34a2738

Add files via upload

e4bfeb8

ritalyu17 marked this pull request as ready for review December 16, 2024 08:11

ritalyu17 requested a review from AdrianSosic as a code owner December 16, 2024 08:11

sgbaird reviewed Dec 16, 2024

View reviewed changes

Update docstring and clean up some code

5c2d8c0

AVHopp requested changes Jan 8, 2025

View reviewed changes

ritalyu17 added 5 commits January 22, 2025 04:02

Merge branch 'emdgroup:main' into Hardness

b8d8cef

Merge branch 'emdgroup:main' into Hardness

9b726d3

Merge branch 'emdgroup:main' into Hardness

fd7a289

Merge branch 'emdgroup:main' into Hardness

567a34c

Update __init__.py

c2eb1fa

AdrianSosic reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardness benchmark #440

Hardness benchmark #440

ritalyu17 commented Dec 3, 2024 •

edited

Loading

CLAassistant commented Dec 3, 2024 •

edited

Loading

ritalyu17 commented Dec 16, 2024 •

edited

Loading

sgbaird Dec 16, 2024

ritalyu17 Dec 17, 2024

AVHopp Jan 8, 2025

AVHopp commented Dec 19, 2024

AVHopp left a comment

AVHopp Jan 8, 2025

AVHopp Jan 8, 2025

ritalyu17 Jan 24, 2025

AVHopp Jan 8, 2025

ritalyu17 Jan 25, 2025

AVHopp Jan 8, 2025

ritalyu17 Jan 25, 2025

AVHopp Jan 8, 2025

ritalyu17 Jan 25, 2025

AVHopp commented Jan 28, 2025

ritalyu17 commented Jan 28, 2025

AdrianSosic left a comment

AdrianSosic commented Feb 25, 2025

ritalyu17 commented Feb 25, 2025 via email

		)


		# IMPORT AND PREPROCESS DATA------------------------------------------------------------------------------

Hardness benchmark #440

Are you sure you want to change the base?

Hardness benchmark #440

Conversation

ritalyu17 commented Dec 3, 2024 • edited Loading

CLAassistant commented Dec 3, 2024 • edited Loading

ritalyu17 commented Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVHopp commented Dec 19, 2024

AVHopp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVHopp commented Jan 28, 2025

ritalyu17 commented Jan 28, 2025

AdrianSosic left a comment

Choose a reason for hiding this comment

AdrianSosic commented Feb 25, 2025

ritalyu17 commented Feb 25, 2025 via email

ritalyu17 commented Dec 3, 2024 •

edited

Loading

CLAassistant commented Dec 3, 2024 •

edited

Loading

ritalyu17 commented Dec 16, 2024 •

edited

Loading