Fix result representation for TDM programs #493

thisac · 2020-11-26T22:19:44Z

Context:
Currently the representation of the Result object doesn't work for TDM programs due to the samples having dimension 3 (shots, spatial modes, timebins) instead of 2.

Description of the Change:
Quick fix that assumes that it's a TDM program if the samples dim is 3, and thus prints the correct representation.

Benefits:
Just writing eng.run(tdm_prog) or res = eng.run(tdm_prog); print(res) works, and actually prints something useful.

Possible Drawbacks:
Might not be the nicest solution, but since Result doesn't know what type of program the samples are from, I think this works well for now. Might need to be revisited later with some further changes to the Result class (incorporating e.g. where the results are from).

Related GitHub Issues:
None

codecov · 2020-11-26T22:24:23Z

Codecov Report

Merging #493 (6c32270) into master (2aa4e15) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #493   +/-   ##
=======================================
  Coverage   97.85%   97.85%           
=======================================
  Files          70       70           
  Lines        7313     7313           
=======================================
  Hits         7156     7156           
  Misses        157      157

Impacted Files	Coverage Δ
strawberryfields/api/result.py	`100.00% <ø> (ø)`
strawberryfields/engine.py	`96.13% <ø> (ø)`

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 2aa4e15...6c32270. Read the comment docs.

strawberryfields/api/result.py

antalszava · 2020-11-26T22:26:44Z

strawberryfields/api/result.py

+            return "<Result: spatial_modes={}, shots={}, timebins={} contains state={}>".format(
+                modes, shots, timebins, self._is_stateful


Think this would be good to keep the order from the shape. Let me know if the change in order was deliberate though!

Suggested change

return "<Result: spatial_modes={}, shots={}, timebins={} contains state={}>".format(

modes, shots, timebins, self._is_stateful

return "<Result: shots={}, spatial_modes={}, timebins={} contains state={}>".format(

shots, modes, timebins, self._is_stateful

I also think this looks nicer to be honest. I just kept the same form as the current repr (for GBS samples) which starts with modes, and then shots. I might just update it in both places if you also think that makes more sense.

Hmm, think it's good to keep the shape order. As for changing for GBS, not quite sure if that would cause any troubles somewhere (don't think so) 🤔.

antalszava · 2020-11-26T22:27:16Z

strawberryfields/api/result.py

-        shots, modes = self.samples.shape
+        try:
+            shots, modes = self.samples.shape
+        except ValueError:


Think this would work fine for now actually! 💯 Would be great to add a test though.

Thanks @antalszava. I've added a test now! You were a bit too quick (and I maybe tagged you for a review a bit too quickly). 😆

antalszava

Hi @thisac the fix is looking great! A test might be nice and then I think it should be good to go.

antalszava · 2020-11-26T22:35:45Z

tests/api/test_result.py

+        result = Result(samples)
+        print(result)
+        out, err = capfd.readouterr()
+        assert "spatial_modes=3" in out


💯 Might be cool to check the order (not too important though).

I think this is fine (since the order isn't actually that important I would say).

antalszava

Looks good to me! 💯

Co-authored-by: antalszava <antalszava@gmail.com>

josh146

Thanks @thisac - just a small comment regarding the existing try-except logic

.github/CHANGELOG.md

josh146 · 2020-11-27T06:16:02Z

strawberryfields/api/result.py

-    Result: 3 subsystems
-        state: <GaussianState: num_modes=3, pure=True, hbar=2>
-        samples: [[0, 0, 0]]
+    <Result: shots=1, num_modes=3, contains state=True>


Good catch, was this accidentally out of date?

Yes it was, not sure why.

josh146 · 2020-11-27T06:20:44Z

strawberryfields/api/result.py

-            modes, shots, self._is_stateful
+        try:
+            shots, modes = self.samples.shape
+        except ValueError:


I'm wondering if there is something better we can catch here than a ValueError, since this exception class is a very common class that could be raised for a whole different host of (unforseen) reasons.

I'm thinking checking the number of dimensions on the samples should be sufficient, e.g.,

def __repr__(self): if self.samples.ndim == 2: shots, modes = self.samples.shape return "<Result: shots={}, num_modes={}, contains state={}>".format(shots, modes, self._is_stateful) if self.samples.ndim == 3: shots, modes, timebins = self.samples.shape return "<Result: shots={}, spatial_modes={}, timebins={} contains state={}>".format( shots, modes, timebins, self._is_stateful ) return "<Result: contains state={}>"

This has a couple of advantages from what I can see:

It is explicit

It avoids catching ValueError exceptions, which are too general

If there are changes in the future, it will not break printing; it will gracefully fail without attempting to print sample information.

Thanks @josh146. I think this is a good solution as well. Updated it to follow your logic above and added a test.

josh146 · 2020-11-27T06:24:15Z

strawberryfields/engine.py

            [strawberryfields.api.Result, None]: the job result if successful, and
-            ``None`` otherwise
+                ``None`` otherwise


Typically we do not indent continuation lines in the returns statement, it will cause Sphinx to render the block incorrectly. Only continuation lines in the Args: section need to be indented.

Moreover, it's a bit weird that there are square brakets here; it should look like this:

strawberryfields.api.Result, None: the job result if successful or ``None`` otherwise

Oh, I didn't actually know. Fixed now!

Co-authored-by: Josh Izaac <josh146@gmail.com>

josh146

Looks great! 💯

strawberryfields/api/result.py

josh146 · 2020-11-28T12:59:34Z

tests/api/test_result.py

+    def test_unkown_shape_print(self, stateful, capfd):
+        """Test that printing a result object with samples with an unknown shape
+        provides the correct output."""


Co-authored-by: Josh Izaac <josh146@gmail.com>

Fix result repr

916d781

thisac changed the title ~~Fix result repr~~ Fix result representation for TDM programs Nov 26, 2020

thisac requested review from josh146 and antalszava November 26, 2020 22:21

antalszava reviewed Nov 26, 2020

View reviewed changes

strawberryfields/api/result.py Outdated Show resolved Hide resolved

antalszava reviewed Nov 26, 2020

View reviewed changes

thisac added 2 commits November 26, 2020 17:27

Add test

4a505be

update changelog

006adfa

antalszava reviewed Nov 26, 2020

View reviewed changes

Update repr again

094bcfd

antalszava approved these changes Nov 26, 2020

View reviewed changes

Update strawberryfields/api/result.py

20d0ceb

Co-authored-by: antalszava <antalszava@gmail.com>

josh146 requested changes Nov 27, 2020

View reviewed changes

thisac and others added 2 commits November 27, 2020 09:48

Update .github/CHANGELOG.md

ca0c48e

Co-authored-by: Josh Izaac <josh146@gmail.com>

Fixes from code-review

8a1bb78

thisac requested a review from josh146 November 27, 2020 15:00

josh146 approved these changes Nov 28, 2020

View reviewed changes

thisac and others added 2 commits November 30, 2020 11:51

Update strawberryfields/api/result.py

5d311eb

Co-authored-by: Josh Izaac <josh146@gmail.com>

antlr bump test

2666ff8

thisac mentioned this pull request Nov 30, 2020

Remove antlr4 requirement + pin Blackbird>=0.3.0 in setup.py #494

Merged

thisac and others added 2 commits November 30, 2020 16:21

remove antlr bump test

332b7e4

Merge branch 'master' into fix-result-repr

6c32270

thisac merged commit 75e3c26 into master Nov 30, 2020

thisac deleted the fix-result-repr branch November 30, 2020 22:17

thisac self-assigned this Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix result representation for TDM programs #493

Fix result representation for TDM programs #493

thisac commented Nov 26, 2020

codecov bot commented Nov 26, 2020 •

edited

Loading

antalszava Nov 26, 2020

thisac Nov 26, 2020

antalszava Nov 26, 2020

antalszava Nov 26, 2020

thisac Nov 26, 2020

antalszava left a comment

antalszava Nov 26, 2020

thisac Nov 26, 2020

antalszava left a comment

josh146 left a comment

josh146 Nov 27, 2020

thisac Nov 27, 2020

josh146 Nov 27, 2020

josh146 Nov 27, 2020

thisac Nov 27, 2020

josh146 Nov 27, 2020

thisac Nov 27, 2020

josh146 left a comment

josh146 Nov 28, 2020

		return "<Result: spatial_modes={}, shots={}, timebins={} contains state={}>".format(
		modes, shots, timebins, self._is_stateful

Fix result representation for TDM programs #493

Fix result representation for TDM programs #493

Conversation

thisac commented Nov 26, 2020

codecov bot commented Nov 26, 2020 • edited Loading

Codecov Report

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

antalszava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava left a comment

Choose a reason for hiding this comment

josh146 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

josh146 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 26, 2020 •

edited

Loading