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

Support for named benchmarks #484

Merged
merged 4 commits into from
Dec 16, 2016
Merged

Conversation

philpep
Copy link
Contributor

@philpep philpep commented Oct 28, 2016

Related to #481

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 58.522% when pulling baff01d on philpep:benchmark-name into a14d8dc on spacetelescope:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 58.522% when pulling baff01d on philpep:benchmark-name into a14d8dc on spacetelescope:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 58.522% when pulling f1fe09b on philpep:benchmark-name into a14d8dc on spacetelescope:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 58.49% when pulling 81c06eb on philpep:benchmark-name into a14d8dc on spacetelescope:master.

@pv
Copy link
Collaborator

pv commented Nov 26, 2016

I guess this is mostly OK. Comments:

  • I think the attribute name should be the more specific benchmark_name instead of name
  • The benchmark selection now needs to import the whole benchmark suite for each benchmark run, which may be expensive. Would it make sense in get_benchmark_from_name to first try importing the benchmark function based on its name, and only if that fails fall back to scrounging with disc_benchmarks?

@pv pv added the needs-work The PR cannot be merged as is, further work required (explained in comments) label Nov 26, 2016
  * move benchmark discovery and instanciation related code in
    disc_benchmark()
  * rename from_name() to get_benchmark_from_name() outside of Benchmark
    class
  * use disc_benchmark() in get_benchmark_from_name(), less optimized
    version but more readable and no more rely on benchmark name.

Related to airspeed-velocity#481
@philpep philpep force-pushed the benchmark-name branch 2 times, most recently from 011b53f to 56c3e2c Compare December 16, 2016 13:59
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 72.241% when pulling 56c3e2c on philpep:benchmark-name into cfe3d3e on spacetelescope:master.

This allow to control the full name of a benchmark function

Related to airspeed-velocity#481
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 72.241% when pulling 3f14e67 on philpep:benchmark-name into cfe3d3e on spacetelescope:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 72.241% when pulling 3f14e67 on philpep:benchmark-name into cfe3d3e on spacetelescope:master.

@philpep
Copy link
Contributor Author

philpep commented Dec 16, 2016

Sorry for the delay.

Pushed new commits with benchmark_name attribute instead of name.

The current implementation of benchmark discovery by name use a generator, so it doesn't always import the whole suite. Nevertheless I re-implemented direct import of benchmark in 9fb848e
I'd prefer to write it shorter but didn't find a way. My opinion is that this is not worth to have this complexity since import of the benchmark suite should be insignificant comparing to the benchmark run. Let me know if you prefer to drop it and I'll update the PR.

Thanks !

@pv
Copy link
Collaborator

pv commented Dec 16, 2016 via email

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 72.283% when pulling 9fb848e on philpep:benchmark-name into cfe3d3e on spacetelescope:master.

@pv
Copy link
Collaborator

pv commented Dec 16, 2016 via email

@philpep
Copy link
Contributor Author

philpep commented Dec 16, 2016

Good catch ! I updated the commit.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 72.283% when pulling fe0e2d3 on philpep:benchmark-name into cfe3d3e on spacetelescope:master.

@pv pv removed the needs-work The PR cannot be merged as is, further work required (explained in comments) label Dec 16, 2016
@pv pv merged commit 390145a into airspeed-velocity:master Dec 16, 2016
@philpep philpep deleted the benchmark-name branch March 20, 2017 10:55
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.

None yet

3 participants