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

BUG: Add a case in p4p_plugin_component.py for Numpy integers. #1120

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

BUG: Add a case in p4p_plugin_component.py for Numpy integers. #1120

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
b29380e
Add a case in p4p_plugin_component.py for Numpy integers.
slactjohnson Oct 16, 2024
26640a6
Update test_send_new_value for np.integer
slactjohnson Nov 1, 2024
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
2 changes: 2 additions & 0 deletions pydm/data_plugins/epics_plugins/p4p_plugin_component.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,8 @@ def send_new_value(self, value: Value) -> None:
self.new_value_signal[str].emit(new_value)
elif isinstance(new_value, dict):
self.new_value_signal[dict].emit(new_value)
elif isinstance(new_value, np.integer):
self.new_value_signal[int].emit(int(new_value))
else:
raise ValueError(f"No matching signal for value: {new_value} with type: {type(new_value)}")
# Sometimes unchanged control variables appear to be returned with value changes, so checking against
Expand Down
14 changes: 9 additions & 5 deletions pydm/tests/data_plugins/test_p4p_plugin_component.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ def generate_control_variables(value):
1,
),
(NTEnum().wrap({"index": 0, "choices": ["YES", "NO", "MAYBE"]}), False, 0, 2),
(NTScalar("i").wrap({"value": np.int32(10)}), False, np.int32(10), 1),
Copy link
Collaborator

@nstelter-slac nstelter-slac Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it seems like NTScalar.wrap converts the np.int32 into just a python int and it gets passed into the testcase, which is b4 the testcase can try to test it. which also means the if on line 82 is not called, (u can add prints to see this and run pytest with -s arg to see print output in terminal)

if we change line 60 to not require a Value type, by having just
value_to_send,
we can actually pass the raw np.int32 into the testcase, but then the testcase fails since send_new_value function expects a Value type (which doesn't seem to support having a wrapped np.integer type, it just converts to python int)

im curious to debug what happens when using this fix with the real pydm screen, and why it doesnt hit this issue? Do you have a screen calling a PV using an np.integer we could use to debug?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also so sorry for long delay on this review!

],
)
def test_send_new_value(
Expand Down Expand Up @@ -78,11 +79,14 @@ def receive_signal(value_name: str, value_received: object):
nonlocal signals_received
signals_received += 1

expected_value_type = type(value_to_send.value)
if isinstance(value_to_send.value, list):
expected_value_type = np.ndarray
elif "NTEnum" in value_to_send.getID():
expected_value_type = int
if isinstance(value_to_send, np.integer):
expected_value_type = type(np.integer)
else:
expected_value_type = type(value_to_send.value)
if isinstance(value_to_send.value, list):
expected_value_type = np.ndarray
elif "NTEnum" in value_to_send.getID():
expected_value_type = int

p4p_connection.new_value_signal[expected_value_type].connect(functools.partial(receive_signal, "value"))
p4p_connection.lower_alarm_limit_signal.connect(functools.partial(receive_signal, "low_alarm_limit"))
Expand Down
Loading