Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Barycentric subdivision #13

Merged
merged 10 commits into from
Sep 13, 2024
Merged

Barycentric subdivision #13

merged 10 commits into from
Sep 13, 2024

Conversation

rballeba
Copy link
Contributor

@rballeba rballeba commented Sep 6, 2024

The pull requests allows one to test the trained neural networks on the barycentric subdivisions of the original test dataset specifying a maximum number of barycentric subdivisions to perform.

.gitignore Outdated Show resolved Hide resolved
Copy link
Contributor

@danielbinschmid danielbinschmid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good. Just some minor questions

)

# add benchmarking results
results.add(data=out[0], config=config)
results.add(data=out[0][0], config=config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this still work for every configuration? Why is the result array now two-dimensional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw the edit in run_experiment.py. I see now that we add a result for every barycentric subdivision. Shouldn't the line be something like results.add(data=out[idx][0], config=config) where idx is the index of the barycentric subdivision then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! True! Good catch!

Copy link
Contributor

@danielbinschmid danielbinschmid Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, it looks better now in 25ff2a0

One more thought:
How is it intended to evaluate the results of the barycentric subdivisions? I think we need to add information about that an added benchmark corresponds to its relevant subdivision configuration in the ResultCollection.

Since the subdivision configuration it is not included in the ConfigExperimentRun, I think it can be done by adding another argument to the add method in the ResultCollection class (needs to also be handled in the save_result method).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMW.

.gitignore Outdated Show resolved Hide resolved
@danielbinschmid
Copy link
Contributor

We also need to merge the new commits in main to this branch

@danielbinschmid danielbinschmid merged commit 5bc90ca into main Sep 13, 2024
2 checks passed
@danielbinschmid danielbinschmid deleted the barycentric_subdivision branch September 14, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants