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

Process wires overriding #1010

Merged
merged 17 commits into from
Jan 19, 2021
Merged

Process wires overriding #1010

merged 17 commits into from
Jan 19, 2021

Conversation

chaserileyroberts
Copy link
Contributor

@chaserileyroberts chaserileyroberts commented Jan 18, 2021

Context:
Increase speed of wire subset method.

Description of the Change:
Previously, the operation Wires.subset was around 20% of the total tape construction time. That has been cut down to just 9% by bypassing the _process method.

Benchmarks from my laptop.

$ asv compare master process_wires

All benchmarks:

       before           after         ratio
     [599e3186]       [360b8357]
     <master>         <process_wires>
       6.06±0.1ms      5.58±0.08ms     0.92  asv.core_suite.CircuitEvaluation.time_circuit(10, 3)
       11.3±0.2ms       10.3±0.4ms    ~0.91  asv.core_suite.CircuitEvaluation.time_circuit(10, 6)
-      16.4±0.4ms       14.3±0.1ms     0.87  asv.core_suite.CircuitEvaluation.time_circuit(10, 9)
      1.88±0.07ms      1.80±0.05ms     0.96  asv.core_suite.CircuitEvaluation.time_circuit(2, 3)
      2.61±0.05ms      2.59±0.06ms     1.00  asv.core_suite.CircuitEvaluation.time_circuit(2, 6)
      3.52±0.06ms      3.31±0.09ms     0.94  asv.core_suite.CircuitEvaluation.time_circuit(2, 9)
      3.19±0.07ms      2.91±0.03ms     0.91  asv.core_suite.CircuitEvaluation.time_circuit(5, 3)
      5.36±0.08ms      4.97±0.04ms     0.93  asv.core_suite.CircuitEvaluation.time_circuit(5, 6)
       7.25±0.1ms       6.69±0.1ms     0.92  asv.core_suite.CircuitEvaluation.time_circuit(5, 9)
       7.94±0.2ms       8.02±0.3ms     1.01  asv.core_suite.GradientComputation.time_gradient_autograd(2, 3)
      22.2±0.08ms       21.8±0.6ms     0.98  asv.core_suite.GradientComputation.time_gradient_autograd(2, 6)
       41.4±0.6ms         40.9±1ms     0.99  asv.core_suite.GradientComputation.time_gradient_autograd(5, 3)
          146±4ms          144±3ms     0.98  asv.core_suite.GradientComputation.time_gradient_autograd(5, 6)
       11.4±0.3ms       11.2±0.1ms     0.98  asv.core_suite.GradientComputation.time_gradient_tf(2, 3)
       27.9±0.3ms       27.9±0.5ms     1.00  asv.core_suite.GradientComputation.time_gradient_tf(2, 6)
       46.9±0.5ms         47.2±1ms     1.01  asv.core_suite.GradientComputation.time_gradient_tf(5, 3)
          155±3ms          155±1ms     1.00  asv.core_suite.GradientComputation.time_gradient_tf(5, 6)
      8.23±0.09ms       8.00±0.1ms     0.97  asv.core_suite.GradientComputation.time_gradient_torch(2, 3)
       21.5±0.5ms       21.8±0.2ms     1.01  asv.core_suite.GradientComputation.time_gradient_torch(2, 6)
         40.8±1ms       41.1±0.3ms     1.01  asv.core_suite.GradientComputation.time_gradient_torch(5, 3)
          147±2ms          144±1ms     0.98  asv.core_suite.GradientComputation.time_gradient_torch(5, 6)

Benefits:
Wire construction is faster

Possible Drawbacks:
None

Related GitHub Issues:
#967

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@codecov
Copy link

codecov bot commented Jan 18, 2021

Codecov Report

Merging #1010 (ca61ba9) into master (599e318) will increase coverage by 0.00%.
The diff coverage is 92.85%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1010   +/-   ##
=======================================
  Coverage   97.89%   97.89%           
=======================================
  Files         151      151           
  Lines       11074    11076    +2     
=======================================
+ Hits        10841    10843    +2     
  Misses        233      233           
Impacted Files Coverage Δ
pennylane/wires.py 98.68% <92.85%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 599e318...ca61ba9. Read the comment docs.

@chaserileyroberts chaserileyroberts changed the title Process wires Process wires overriding Jan 18, 2021
@chaserileyroberts
Copy link
Contributor Author

Codecov is being mean again lol

@josh146
Copy link
Member

josh146 commented Jan 19, 2021

Looking at the CI logs:

 ==> GitHub Actions detected.
->  Issue detecting commit SHA. Please run actions/checkout with fetch-depth > 1 or set to 0

🤔 So it looks like codecov is using the wrong commit to generate the codecov/patch check. Note, though, that this check is not a required check!

@chaserileyroberts
Copy link
Contributor Author

Please run actions/checkout with fetch-depth > 1 or set to 0

Is this something we can fix?

@josh146
Copy link
Member

josh146 commented Jan 19, 2021

You could try editing the .github/workflows/tests.yml checkout action in this branch to have the following, and see if the error disappears from the logs:

  with:
    fetch-depth: 0

I'm not sure what this does, but the log tells us to do it 😆

Oh there are more details here: https://github.com/actions/checkout#fetch-all-history-for-all-tags-and-branches

@@ -76,8 +76,11 @@ class Wires(Sequence):
and hence interpreted as a single wire.
"""

def __init__(self, wires):
self._labels = _process(wires)
def __init__(self, wires, _override=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

I have never seen "private" kwargs...is there a convention about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to discourage users from using it.

subset = [self[i] for i in indices]
return Wires(subset)
subset = tuple(self._labels[i] for i in indices)
return Wires(subset, _override=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@mariaschuld mariaschuld left a comment

Choose a reason for hiding this comment

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

Awesome: small, safe changes that lead to speedups :)

def test_equal_to_tuple(self):
assert Wires([1, 2, 3]) == (1, 2, 3)
assert Wires([1, 2, 3]) != (1, 5, 3)
assert (1, 5, 3) != Wires([1, 2, 3])
Copy link
Contributor

Choose a reason for hiding this comment

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

Also nice!

@chaserileyroberts chaserileyroberts merged commit f5d465f into master Jan 19, 2021
@chaserileyroberts chaserileyroberts deleted the process_wires branch January 19, 2021 17:37
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