Issue-132: Revert API changes introduces with #86

[Guzz-T]

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:

• Add a data class that contains the parameters, calculations and visibilities
• Keep the seperation of the data class and the read/write interface
• Provide a new "Luxtronik" class that combines the data and the interface

This allows both the old and the new API to be used.

Fix #132

NOTE: The linter added some format changes.

[github-actions]

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 💤	0 ❌	0 🔥	0.680s ⏱️

[Guzz-T]

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.

[gerw]

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.

Reference

[Guzz-T]

But this is fixed by moving Parameters(True) to the function body in the latest patch, right?

Yes, that has already been adjusted.

[Guzz-T]

Btw: True is the default argument of Parameters() and can be omitted.

Done

[gerw]

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:

• The two copy() methods do not work as intended.
• The deepcopy() are quite slow. On my (rather fast) machine, they need 0.01 seconds.
• The 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

[Guzz-T]

The deepcopy() from the copy module is good enough and we do not need to implement our own deepcopy()

I think that we can also drop new() by using

Done

[gerw]

I think that I am (almost) satisfied with the changes in __init__.py. (I didn't check the other files yet.)

Thanks for the great work!

[gerw]

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]

@Guzz-T : Can you please rebase to main and squash all these commits to a single one?

Done

[Bouni]

LGTM I highly like the fact that we unbreak the API 😄

[gerw]

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?

I am looking forward to merging this.

[Guzz-T]

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.

[Bouni]

I think everything is resolved and ready. I'll merg it.