Closed Guzz-T closed 5 months ago
Coverage Report
File Stmts Miss Cover Missing luxtronik __init__.py 179 134 25% 39–51, 61–63, 70–74, 77, 81–86, 90–94, 105–108, 116–118, 125–127, 134–136, 143–145, 154, 162–164, 167–170, 173–176, 179–195, 198–212, 215–231, 234–248, 252–254, 258–259, 263–264, 275–277, 280, 283, 286, 289, 292–295, 298–301 __main__.py 21 21 0% 3–49 datatypes.py 275 1 99% 82 discover.py 40 34 15% 21–69 luxtronik/scripts dump_changes.py 44 44 0% 5–85 dump_luxtronik.py 27 27 0% 5–52 TOTAL 673 261 61%
Tests | Skipped | Failures | Errors | Time |
---|---|---|---|---|
120 | 0 :zzz: | 0 :x: | 0 :fire: | 0.680s :stopwatch: |
Unfortunately read_parameters(self, parameters=Parameters(True)
doesn't work reliably. The first time an instance is created. From the second call onwards, the reference is still stored, which means that no new instance is created.
But this is fixed by moving Parameters(True)
to the function body in the latest patch, right?
Btw: True
is the default argument of Parameters()
and can be omitted.
The reason seems to be the following, I didn't know that either.
Python’s default arguments are evaluated once when the function is defined, not each time the function is called (like it is in say, Ruby). This means that if you use a mutable default argument and mutate it, you will and have mutated that object for all future calls to the function as well.
But this is fixed by moving
Parameters(True)
to the function body in the latest patch, right?
Yes, that has already been adjusted.
Btw:
True
is the default argument ofParameters()
and can be omitted.
Done
The copy()
method does a shallow copy. We also (instead?) need a deep copy:
class LuxtronikData:
[...]
@classmethod
def deepcopy(cls, data):
return LuxtronikData(deepcopy(data.parameters), deepcopy(data.calculations), deepcopy(data.visibilities))
With this addition, consider this snippet:
from luxtronik import LuxtronikData
import copy
import timeit
l = LuxtronikData()
l.parameters.get(0).raw = 1
print("l.parameters.get(0).raw =", l.parameters.get(0).raw)
l2 = LuxtronikData.copy(l)
l3 = copy.copy(l)
l4 = copy.deepcopy(l)
l5 = LuxtronikData.deepcopy(l)
print("===")
l.parameters.get(0).raw = 2
print("l.parameters.get(0).raw =", l.parameters.get(0).raw)
print("l2.parameters.get(0).raw =", l2.parameters.get(0).raw)
print("l3.parameters.get(0).raw =", l3.parameters.get(0).raw)
print("l4.parameters.get(0).raw =", l4.parameters.get(0).raw)
print("l5.parameters.get(0).raw =", l5.parameters.get(0).raw)
print("===")
num = 100
s = """
from luxtronik import LuxtronikData
import copy
l = LuxtronikData()
"""
print(timeit.timeit('l2 = LuxtronikData.copy(l)', setup=s, number=num)/num)
print(timeit.timeit('l3 = copy.copy(l)', setup=s, number=num)/num)
print(timeit.timeit('l4 = copy.deepcopy(l)', setup=s, number=num)/num)
print(timeit.timeit('l5 = LuxtronikData.deepcopy(l)', setup=s, number=num)/num)
Output for me:
l.parameters.get(0).raw = 1
===
l.parameters.get(0).raw = 2
l2.parameters.get(0).raw = 2
l3.parameters.get(0).raw = 2
l4.parameters.get(0).raw = 1
l5.parameters.get(0).raw = 1
===
4.6712099992873845e-06
1.3894300013816973e-06
0.00908147751999877
0.009089312020000762
Thus:
copy()
methods do not work as intended.deepcopy()
are quite slow. On my (rather fast) machine, they need 0.01 seconds.deepcopy()
from the copy
module is good enough and we do not need to implement our own deepcopy()
method.I think that we can also drop new()
by using
class LuxtronikData:
"""
Collection of parameters, calculations and visiblities.
Also provide some high level access functions to their data values.
"""
def __init__(self, parameters=None, calculations=None, visibilities=None, safe=True):
self.parameters = Parameters(safe) if parameters is None else parameters
self.calculations = Calculations() if calculations is None else calculations
self.visibilities = Visibilities() if visibilities is None else visibilities
The
deepcopy()
from thecopy
module is good enough and we do not need to implement our owndeepcopy()
I think that we can also drop
new()
by using
Done
I have made some changes to the other files. Now, I am fine with the PR
@Guzz-T : Can you please rebase to main and squash all these commits to a single one?
@Bouni @kbabioch : What do you think about the PR?
@Guzz-T : Can you please rebase to main and squash all these commits to a single one?
Done
LGTM I highly like the fact that we unbreak the API 😄
IMHO, only two minor things are left. And the commit message "Add a decorator to provide the previous api" might be a copy-and-paste leftover from the last PR?
Now use the title of this pull-request.
I think everything is resolved and ready. I'll merg it.
With #86 we split the data from the read/write interface. However, these changes mean that the existing program needs to be adjusted.
With this pull-request we restore the previous api by:
This allows both the old and the new API to be used.
Fix #132
NOTE: The linter added some format changes.