Closed lneuhaus closed 3 years ago
Merging #566 (d3fb71e) into master (25ab8d4) will not change coverage. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## master #566 +/- ##
=======================================
Coverage 98.36% 98.36%
=======================================
Files 76 76
Lines 8332 8332
=======================================
Hits 8196 8196
Misses 136 136
Impacted Files | Coverage Δ | |
---|---|---|
strawberryfields/tdm/tdmprogram.py | 95.72% <100.00%> (ø) |
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 25ab8d4...d3fb71e. Read the comment docs.
Is the weird behavior of codecov expected? I just changed a few lines, no way to have decreased coverage by 40% ;)
I guess test_tdmprogram.py
needs to be updated too, but otherwise looks good Leo!
Looks good overall! :slightly_smiling_face: Thank you! Indeed, as Luke mentioned, probably this line needs an update.
since SF now uses API versioning, my understanding is this shouldn't be changed unless the pinned API version is also increased as needed. Unless somehow the current API version that SF targets was changed? But maybe someone from platform can clarify this (@jswinarton?)
What I know for sure:
{"max": {"key": value}}
. It's not entirely clear to me what keys the specification allows here apart from "max" in the top-level dict, but the only allowed value types here should be dictionaries (i.e. also not integers). From that perspective the current SF implementation is not aligned with the spec for API version 0.2.0 already (since it expects {"concurrent": 2}
for example), so I believe we are in a gray zone here anyways.Either we admit that the API 0.2.0 spec is wrong and expand it
modes: Union[int, Dict[str, Dict[str, int]]]
modes: Union[int, Dict[str, Union[int, Dict[str, int]]]]
to be consistent with what's being returned by the API anyways, or we take the API 0.2.0 as the ultimate source of truth, in which case I could modify this PR to take that into account.
Definitely curious to get @jswinarton, @gtr8, or @Mandrenkov 's input on this.
- all API versions >= 0.3.0 serve un-nested dicts, so should be in agreement with this patch.
True, but I think this statement is weaker than you think. Here is how the 0.3.0 endpoint is currently implemented:
modes
is not a dictionary (e.g., an integer), return it without modification.modes
is a dictionary and the target starts with simulon
, flatten the modes into a Dict[str, Any]
and return it.modes
is a dictionary and the target does not start with simulon
, return an empty dictionaryIf you try and request the device specifications for a TD2 device using the 0.3.0 endpoint, you will get an empty dictionary.
- API version 0.2.0 (the one SF currently requests (see here) specifies a nested dictionary with structure
{"max": {"key": value}}
. It's not entirely clear to me what keys the specification allows here apart from "max" in the top-level dict, but the only allowed value types here should be dictionaries (i.e. also not integers). From that perspective the current SF implementation is not aligned with the spec for API version 0.2.0 already (since it expects{"concurrent": 2}
for example), so I believe we are in a gray zone here anyways.
The platform 0.2.0 API specification does indeed state that modes
is either an integer or a dictionary of the form {"max": {"key": value}}
; however, the 0.2.0 implementation does not perform any validation in this regard. So, if you request the TD2 device specifications using the 0.2.0 endpoint, you will receive
"modes": {
"equal": {
"concurrent": 2,
"spatial": 1
},
"max": {
"temporal": 100
}
}
Note the "equal"
key above; this is exactly what the TD2 device specifications return today.
Either we admit that the API 0.2.0 spec is wrong and expand it
- from
modes: Union[int, Dict[str, Dict[str, int]]]
- onto
modes: Union[int, Dict[str, Union[int, Dict[str, int]]]]
to be consistent with what's being returned by the API anyways, or we take the API 0.2.0 as the ultimate source of truth, in which case I could modify this PR to take that into account.
I think performing either change is not great for semantic versioning so we should do the one that leaves the API in the most desirable state. That is, we either
Dict[str, int]
, orI'm not sure which of these options is more convenient for Strawberry Fields.
@Mandrenkov and I have run a test with this branch - it is now in line with how the platform presents the modes
data, from API version 0.2.0 onwards. So
Fix the 0.2.0 API implementation to transform the TD2 device spec into a Dict[str, int]
is what both the platform and this PR went with, so I suggest to simply merge this PR and move on.
to minimize the TD2 downtime
TD2 is effectively down until this fix is applied since a platform upgrade around January (introduction of device API) - therefore I suggest we merge this now in order to be sure to integrate this behavior into the next Strawberry Fields release.
Okay, sounds good!
Context: Device specifications for TDM programs that are exposed by Xanadu's device API used to include a field
modes
with data likeThis format was recently changed into (ex generis):
Description of the Change: This PR brings Strawberry Fields up to date with the new format.
Benefits: TDM programs can run on Xanadu hardware.
Possible Drawbacks: None.
Related GitHub Issues: None.