Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev: specification of the interface of the Sweep class #657

Open
asarhaddon opened this issue Jul 9, 2023 · 0 comments
Open

dev: specification of the interface of the Sweep class #657

asarhaddon opened this issue Jul 9, 2023 · 0 comments
Labels
enhancement New feature or request

Comments

@asarhaddon
Copy link
Contributor

Hello.

Some changes I have submitted for the Sweep class have been reverted (without much comments), I have also reverted the related changes in tests (6eb24f2, d09b55e, dbea311). And now python (rightfully) complains because the default value Properties() for the property field of the dataclass Sweep is mutable.
This dance (and lots of anterior commits) could probably be avoided by first explicitly defining the intent of this string, either in comments or doc strings.
Here are a few questions that a newcomer like me would like to see answered in the code.

  • Why a separate Properties class? How are its fields different from the other ones in the Sweep class?

  • Is it possible to use private fields in the usual way? Namely:

def set_start(self, start): with self._lock: self._start = start
def start(self): return self._start
def span(self): return self._end - self._start

Such declarations make evident that other classes should not modify self.start or self.span (which are functions) and self._start (which is private).

  • I suggest to remove the @property syntactic sugar, which trades some confusion for a few keystrokes. The code does not contain much comments, it may be useful to search all calls to sweep.set_start. Also, the fact that implementations in the Sweep class do use hidden accessors is surprising, they are intended for public interfaces.

  • What is the intent of self.lock? In some places, it seems that it should be a private field of the Sweep class, used in setters like above, in order to ensure that any change of the field value is protected. But...

    DeviceSettings.py sets sweep.points without locking.

    SweepSettings.py locks while calling self.app.vna.setTXPower(...), which does not seem to affect self.app.sweep.

  • Comparison of Sweep classes should probably use a documented function instead of redefining the python equality, so that it is easy to find where it is called from. For example, this portion of SweepWorker.py

with self.app.sweep.lock:
    sweep = self.app.sweep.copy()
if sweep != self.sweep:  # parameters changed
    self.sweep = sweep

is hard to understand even with a comment.
May it be simplified like this ? (the answer probably depends on the intent of the lock)

if self.app.sweep != self.sweep:  # parameters changed
    self.sweep = self.app.sweep.copy()

Thanks

@asarhaddon asarhaddon added the enhancement New feature or request label Jul 9, 2023
zarath pushed a commit that referenced this issue Jul 30, 2023
* style, Sweep.py: remove a double negation

* style, NanoVNASaver.py: simplify sweepSource computation

* Sweep.py: add getters and setters for private fields

Beware that this commit removes a lock from
SweepSettings.update_tex_power, and adds one to
DeviceSettings.updatecustomPoint.
Both changse may be incorrect, depending on the role of the lock
(issue #657).

Follows: 6eb24f2 d09b55e dbea311

Since d09b55e, the Properties.name class attribute is overriden by
each assignment to the properties.name instance attribute.
This is most probably unwanted.

This commit

 * removes @DataClass, which is confusing as some attributes are
   managed because of the lock.
   Because of this, it has to restore __repr__ and __eq__.
 * provides getters and setters for private attributes, and
   protects each update by a thread lock
 * adds a regression test for the bug fixed by d09b55e (immutable
   properties).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant