Closed Aaron-Robertson closed 3 years ago
Merging #562 (2847d60) into master (ebcca23) will increase coverage by
0.00%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #562 +/- ##
=======================================
Coverage 98.36% 98.36%
=======================================
Files 76 76
Lines 8325 8332 +7
=======================================
+ Hits 8189 8196 +7
Misses 136 136
Impacted Files | Coverage Δ | |
---|---|---|
strawberryfields/backends/tfbackend/__init__.py | 73.33% <ø> (-3.14%) |
:arrow_down: |
strawberryfields/backends/tfbackend/backend.py | 100.00% <100.00%> (ø) |
|
strawberryfields/backends/tfbackend/circuit.py | 95.89% <100.00%> (+0.03%) |
:arrow_up: |
strawberryfields/backends/tfbackend/ops.py | 98.05% <100.00%> (-0.01%) |
:arrow_down: |
strawberryfields/backends/tfbackend/states.py | 98.47% <100.00%> (+0.02%) |
:arrow_up: |
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 ebcca23...2847d60. Read the comment docs.
@josh146 I'm just coming back to this one today and it looks to me like the simple change to ops.def_type
is not a valid proof of concept anymore (errors coming back from thewalrus
). Want me to push up that line change here so you can take a look at test errors?
Want me to push up that line change here so you can take a look at test errors?
Yes, that would be helpful!
@Aaron-Robertson do you see issues if you replace the built-in type with another precision of complex number? (I doubt things would work with floats, a lot of things under the hood leverage complex numbers)
@co9olguy gotcha, that makes sense. I was confused by the float64
use case from the issue/related comment, but see now it's in reference to the Tensor rather than ops.def_type
. And yep, complex128
does complete in tests.
TF's DType
only defines complex128
and complex64
though, so maybe just allow user to specify a bool mem_intense
or something similar?
TF's DType only defines complex128 and complex64 though, so maybe just allow user to specify a bool mem_intense or something similar?
Nice idea, but I wonder if there is a need for that level of control? From feedback I've heard in the past, I would say it's sufficient to stick with complex64
as the default dtype, but allows users to change it to complex128
if they desire (and raise an error if float64
is passed, for example)
Nice idea, but I wonder if there is a need for that level of control?
I just meant mem_intense=True
=> complex128
and mem_intense=False
=> complex64
as default.
I would say it's sufficient to stick with complex64 as the default dtype, but allows users to change it to complex128 if they desire
Can do, I thought it would be confusing if there's a dtype arg but the only accepted values are complex64
and complex128
. Maybe call it complex_dtype?
Oh I see! Thanks for the clarification @Aaron-Robertson. I'm happy with either solution, but tend to lean more towards the complex_dtype
solution, since it makes it more explicit which dtype is being used by the backend
Since having mem_intense
as an argument would already require a docstring describing what exactly that means, I'd be in favour of instead just having dtype
as the name, saying the acceptable values in the docstring (i.e., the two complex variants), and throwing an direct error message if any other dtype is received
Alright, made enough bandwidth for this one in the next few days (~and probably want to clarify before I get too much deeper~ I just went for it 😄 ). Do we want to follow existing patterns for backend args? Essentially add the optional dtype to each tfbackend/ops.py
function and pass down from circuit/states?
~I can push~ Pushed what I have, but that's where I landed after testing a couple other approaches. Happy to switch it up too if there's a better recommendation!
Could use a recommendation on testing as well. Attempted additions to conftest
witout much success.
Tried a couple cleanups and oddly ran into pipeline test failures that didn't occur locally. Let's merge this one and I can make a cleanup PR for some of the todos in the tf backend, and to similarly replace def_type
with dtype
param for the fock backend 😄
Context: Resolves #486
Description of the Change: Make tensorflow dtype adjustable by user by adding a
dtype
optional argument tobackend_options
. Follows existing patterns for optional arguments.Benefits: User can adjust
dtype
, i.e.:Possible Drawbacks: N/A?
Related GitHub Issues:
486