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

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ ritalyu17
❌ pre-commit-ci[bot]
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

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

benchmarks/domains/__init__.py

benchmarks/domains/Hardness.py

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

ritalyu17 · 2025-03-03T04:14:42Z

Hi @AdrianSosic, I have updated both benchmarks to match the coding convention with other scripts in the repository.

There is one thing that I couldn't quite figure out with, benchmark for transfer learning. In transfer learning, I want to work with different initial data sizes. But, initial_data argument is only used in simulate_scenarios, is there a way to do this elegantly? (Line 202-216 in Hardness benchmark)

AdrianSosic · 2025-03-03T07:35:57Z

Hi @AdrianSosic, I have updated both benchmarks to match the coding convention with other scripts in the repository.

There is one thing that I couldn't quite figure out with, benchmark for transfer learning. In transfer learning, I want to work with different initial data sizes. But, initial_data argument is only used in simulate_scenarios, is there a way to do this elegantly? (Line 202-216 in Hardness benchmark)

Hi @ritalyu17, thanks for pinging me. Yes, there is an easy way to handle the initial_data problem: instead of passing a fixed dataset, just pass an iterable of datasets and omit the n_mc_iterations argument. That way, one MC run will be performed for each dataset you pass.

Once you've included the change, ping me again and I'll have a look at the code 👍🏼

AdrianSosic · 2025-03-04T12:00:20Z

Hi @ritalyu17, let me know when changes are incorporated and the branch is rebased 👍🏼

for more information, see https://pre-commit.ci

ritalyu17 · 2025-03-05T02:37:28Z

Hi @AdrianSosic, thanks for the suggestions. This pull request is ready for review.

AdrianSosic

Hi @ritalyu17, thx for the draft. Here my first comments

AdrianSosic · 2025-03-12T07:40:21Z

benchmarks/domains/Hardness.py

@@ -0,0 +1,266 @@
+# Hardness benchmarking, a maximization task on experimental hardness dataset.


You have a longer explanation in the __main__ section of the script. That should go here, and you can then print it from main via the __doc__ attribute of the file. Overall, it should become very clear from the text what is the goal of this benchmark. Details that are not relevant to understand the overall task, e.g. how exactly the data is loaded (for example, that you consider only contexts with more then 5 points etc) should not be mentioned here but in their respective code section. For example, for the data loading, you'd need to add some data loading function whose docstring/comments explain it.

AdrianSosic · 2025-03-12T07:41:05Z

benchmarks/domains/Hardness.py

+)
+
+# Set up directory and load datasets
+home_dir = os.getcwd()


The data should not live here but in a separate folder, probably under /benchmarks/data/hardness/

AdrianSosic · 2025-03-12T07:41:27Z

benchmarks/domains/Hardness.py

+home_dir = os.getcwd()
+# Materials Project (MP) bulk modulus dataset
+df_mp = pd.read_csv(
+    os.path.join(home_dir, "benchmarks", "domains", "mp_bulkModulus_goodOverlap.csv"),


avoid os.path. Please use pathlib.Path instead for path manipulations

AdrianSosic · 2025-03-12T07:42:44Z

benchmarks/domains/Hardness.py

+    ConvergenceBenchmarkSettings,
+)
+
+# Set up directory and load datasets


General comment: you execute all these commands in the main scope of the module, which is suboptimal. Please split the logic up into meaningful pieces and extract them into reasonable functions, e.g. one for data loading, one for data pre-processing (spline interpolation) etc

AdrianSosic · 2025-03-12T07:43:23Z

benchmarks/domains/Hardness.py

+    composition_subset = df_exp[df_exp["composition"] == composition_i]
+    # Sort the data by load
+    composition_subset = composition_subset.sort_values(by="load")
+    composition_subset = composition_subset.drop_duplicates(subset="load")


what if there are multiple identical load values where the other column values differ?

AdrianSosic · 2025-03-12T07:49:48Z

benchmarks/domains/Hardness.py

+hardness_benchmark = ConvergenceBenchmark(
+    function=hardness,
+    settings=benchmark_config,
+    optimal_target_values=None,


How come we don't know the optimal value? This should be clear from the dataset, no?

AdrianSosic · 2025-03-12T07:50:05Z

benchmarks/domains/Hardness.py

+    ax.set_xlabel("Hardness")
+    ax.set_ylabel("Frequency")
+    ax.set_title("Integrated Hardness Distribution")
+    ax.grid()


plt.show() is missing

AdrianSosic · 2025-03-12T07:50:38Z

benchmarks/domains/Hardness.py

file should be called hardness.py

AdrianSosic · 2025-03-12T07:51:10Z

benchmarks/domains/__init__.py

Changelog entry is missing

AdrianSosic · 2025-03-12T07:53:44Z

benchmarks/domains/__init__.py

Some general comment: the basic code requirements are not yet met, because it seems you haven't installed the pre-commit hooks while developing. Please:

Run the hooks (you can also trigger them manually via pre-commit run --all-files) and fix the problems

Run mypy and fix the typing issues.

You can also find more information here: https://emdgroup.github.io/baybe/stable/misc/contributing_link.html

AVHopp reviewed Dec 9, 2024

View reviewed changes

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

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

AVHopp requested changes Jan 8, 2025

View reviewed changes

AdrianSosic reviewed Feb 7, 2025

View reviewed changes

ritalyu17 closed this Mar 1, 2025

ritalyu17 force-pushed the Hardness branch from 586abc0 to 70b4978 Compare March 1, 2025 01:34

ritalyu17 reopened this Mar 1, 2025

ritalyu17 and others added 10 commits March 4, 2025 20:33

Add files via upload

a382659

[pre-commit.ci] auto fixes from pre-commit.com hooks

14473d7

for more information, see https://pre-commit.ci

Rename config module to base

0a265ee

Update __init__.py

0a8aed5

[pre-commit.ci] auto fixes from pre-commit.com hooks

59c30ef

for more information, see https://pre-commit.ci

Update import ConvergenceBenchmark

d73ccaf

[pre-commit.ci] auto fixes from pre-commit.com hooks

eaba4f9

for more information, see https://pre-commit.ci

Update NumericalTarget use

b2601d4

Update transfer learning for different initial data size

192b0ce

[pre-commit.ci] auto fixes from pre-commit.com hooks

6d05dcd

for more information, see https://pre-commit.ci

ritalyu17 force-pushed the Hardness branch from 3d783bf to 6d05dcd Compare March 5, 2025 01:35

AdrianSosic reviewed Mar 12, 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 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

ritalyu17 commented Mar 3, 2025 •

edited

Loading

AdrianSosic commented Mar 3, 2025

AdrianSosic commented Mar 4, 2025

ritalyu17 commented Mar 5, 2025

AdrianSosic left a comment

AdrianSosic Mar 12, 2025

AdrianSosic Mar 12, 2025

AdrianSosic Mar 12, 2025

AdrianSosic Mar 12, 2025

AdrianSosic Mar 12, 2025

AdrianSosic Mar 12, 2025

AdrianSosic Mar 12, 2025

AdrianSosic Mar 12, 2025

AdrianSosic Mar 12, 2025

AdrianSosic Mar 12, 2025

		@@ -0,0 +1,266 @@
		# Hardness benchmarking, a maximization task on experimental hardness dataset.

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

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 Mar 3, 2025 • edited Loading

AdrianSosic commented Mar 3, 2025

AdrianSosic commented Mar 4, 2025

ritalyu17 commented Mar 5, 2025

AdrianSosic 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

Choose a reason for hiding this comment

ritalyu17 commented Dec 3, 2024 •

edited

Loading

CLAassistant commented Dec 3, 2024 •

edited

Loading

ritalyu17 commented Dec 16, 2024 •

edited

Loading

ritalyu17 commented Mar 3, 2025 •

edited

Loading