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.

Sign up for GitHub

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

Jump to bottom

FIX: Account for custom scalar types of incoming values #737

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

FIX: Account for custom scalar types of incoming values #737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41c3fa6
FIX: Use base type as channeltype when incoming value is an instance …
Jun 9, 2021
d37a975
TST: Add test for channeltype improvement
Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions pydm/tests/widgets/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import pytest
import json
import numpy as np
import logging
logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -224,6 +225,51 @@ def test_pydmwidget_alarm_severity_changed(qtbot):
assert pydm_label.alarmSeverity == PyDMWidget.ALARM_MAJOR


class FloatSubclass(float):
pass


class StrSubclass(str):
pass


@pytest.mark.parametrize("value, expected_type", [
(123, int),
(123.47, float),
(1e2, float),
(0x1FF, int),
(0b100, int),
(True, bool),
(False, bool),
(np.array([123, 456]), np.ndarray),
("text", str),
(FloatSubclass(0.5), float),
(StrSubclass(0.5), str),
])
def test_pydmwidget_channeltype(qtbot, value, expected_type):
"""
Test the registered channel type on in coming value of a PyDMWritableWidget object.

Expectations:
Channel type is a type of incoming value or its base class.

Parameters
----------
qtbot : fixture
Window for widget testing
value : str
The incoming value from the channel
expected_type: int, float, str, bool, np.ndarray, FloatSubclass, StrSubclass
The type to be registered in channeltype
"""
pydm_label = PyDMLabel()
qtbot.addWidget(pydm_label)

pydm_label.value_changed(value)

assert pydm_label.channeltype == expected_type


@pytest.mark.parametrize("init_channel", [
"CA://MA_TEST",
"",
Expand Down
6 changes: 6 additions & 0 deletions pydm/widgets/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,6 +680,12 @@ def value_changed(self, new_val):
"""
self.value = new_val
self.channeltype = type(self.value)
for base_type in [int, float, str, np.ndarray]:
if self.channeltype != base_type and issubclass(self.channeltype, base_type):
# Leave bool and int separate, we're concerned about user-defined subclasses, not native Python ones
if self.channeltype != bool:
self.channeltype = base_type
break
Comment on lines +683 to +688
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how often code gets called, but wouldn't it be more efficient to test for bool once and then in the loop it can just test if something is a subtype.

     if self.channeltype !=bool:
         for base_type in [int, float, str, np.ndarray]:
              if issubclass(self.channeltype, base_type):
                 self.channeltype = base_type
              break

Notice the check for self.channeltype!=base_type is unnecessary. If it is int then it would pass the subclass check and set channeltype to int which it already was and then break.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @Thrameos's sentiment here. value_changed gets called a lot, maybe more than any other method in all of PyDM, so it pays to make this check as lightweight as possible.

Maybe something like this?

if self.channeltype not in (int, float, bool, str, np.ndarray):
    for base_type in (int, float, str, np.ndarray):
        if issubclass(self.channeltype, base_type): 
            self.channeltype = base_type
        break

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a critical section then skipping the for loop and the implicit loop from "not in" entirely would be the best policy. Instead use a global table cache which would be O(1).

         value_type = type(value)
         self.channeltype = TYPE_TABLE.get(value_type, None)
         if self.channeltype is None:
             # assume we will use the type as is
             self.channeltype = value_type

             # check for user defined subtypes
             for base_type in (int, float, str, np.ndarray):
                if issubclass(self.channeltype, base_type): 
                   self.channeltype = base_type
                   break

             # Cache the result so all future look ups are O(1)
             TABLE_TYPE[value_type] = self.channeltype

The table would be prepopulated with TYPE_TABLE[bool] = bool

Each time it is called we have one hash map look up and a comparison. It it is not found then the search occurs to find the best type. That result is cached so it never has to evaluate the loop for that type again.

if self.channeltype == np.ndarray:
self.subtype = self.value.dtype.type
else:
Expand Down