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

Add Parameters to Benchmarks #103

Merged
merged 5 commits into from
Mar 10, 2024
Merged

Add Parameters to Benchmarks #103

merged 5 commits into from
Mar 10, 2024

Conversation

maxmynter
Copy link
Collaborator

Closes #101

Add the Parameters to a 'BenchmarkRecord' by appending it to the results. Omit the type information from the Variable object and only retain variable name and value.

Add the Parameters to a 'BenchmarkRecord' by appending it to the results.
Omit the type information from the `Variable` object and only retain variable
name and value.
@maxmynter maxmynter marked this pull request as ready for review March 8, 2024 17:39
Copy link
Collaborator

@nicholasjng nicholasjng left a comment

Choose a reason for hiding this comment

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

Great job!

src/nnbench/runner.py Outdated Show resolved Hide resolved
@@ -263,6 +264,7 @@ def run(
"date": datetime.now().isoformat(timespec="seconds"),
"error_occurred": False,
"error_message": "",
"parameters": {**bmdefaults, **bmparams},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this ensure that the defaults are overwritten by the bmparams?

I think something like bmdefaults.update(bmparams) would be easier to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the entires are unpacked in order with the latter overwriting the former in case of colliding keys.

dict.update does not return the merged dict, but just updates inplace.

We couid do dict(bmdefaults, **bmparams) , though I don't think that is easier to read.
Alternatively, do an update on bmdefaults and then pass only that. That is an extra line though and we have a bmdefaults flying around that does not contain the defaults.

Copy link
Collaborator

@nicholasjng nicholasjng Mar 8, 2024

Choose a reason for hiding this comment

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

I think this does what it needs to do: For any variable, if the default is used, it's not in bmparams, so the update does nothing - and for every overridden default, the update() overwrites with the argument given in the params dict.

So the order should be "use defaults, then update with params", which is what I suggested (you can also write bmdefaults | bmparams).

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you need a mental model, it helps to think about what happens once the params are passed into the function - its default keyword arguments are already there, and everything else gets overridden by the incoming parameters. So it is indeed bmdefault.update(bmparams) we're looking for here (you can even use that as the dict value, since we do not explicitly use it again afterwards).

maxmynter and others added 2 commits March 8, 2024 20:11
Co-authored-by: Nicholas Junge <[email protected]>
What's left is to decide what to do with parameter types that aren't as
easily ingestible into a sink as builtin datatypes.
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.

Benchmark results do not contain parameters
2 participants