feat: add signal aliases on SignalGroup

[getzze]

Split from #265
solves #261

Add a keyword argument to SignalGroupDescriptor to specify signal aliases (if the alias is None, no signal is created for this field). The signal aliases table is stored in the SignalGroup class (or subclass).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (33a6f20) to head (cfa8f71).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #299   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         1972      2009   +37     
=========================================
+ Hits          1972      2009   +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codspeed-hq]

CodSpeed Performance Report

Merging #299 will improve performances by 14.18%

_{Comparing getzze:signal-aliases (cfa8f71) with main (33a6f20)}

Summary

⚡ 1 improvements
✅ 65 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	getzze:signal-aliases	Change
⚡	test_emit_time[lambda-2]	128.2 µs	112.3 µs	+14.18%

[getzze]

All tests pass, there is just a small performance regression. This is good to review.

[tlambert03]

at first glance I'm impressed! will review more thoroughly soon. thanks again @getzze

[tlambert03]

hey @getzze, I think this is probably my only question (in an extremely nice PR). I can see why you did it this way as opposed to modifing __getitem__ It allows someone to query both the alias name and the original name. Do you think that's important? (I assume so, since you've been giving it a lot of thought and opted for this).

[getzze]

hey, I think it can be removed, I mean inlined in evented_setattr, as it is only used there. And SignalGroup has one less method.

[tlambert03]

i do think that end-users should also be able to search by alias. but i guess the question is should they no longer be able to search by the original name? i.e. if I remove the method on SignalGroup, then end-users can no longer access the original names for an aliased signal, correct? are we ok with that?

[getzze]

I made a function instead of a method, with a new name get_signal_from_field.

[tlambert03]

hey @getzze, I was able to avoid the slight performance regression with 64e2fb4

i know it was a very small one, but time to set attribute on evented models is probably the benchmark i am most concerned about keeping fast.

[getzze]

I see you had to do something quite convoluted to avoid the performance regression. But I'm glad you managed to make it work.
I don't know how to run the benchmarks in local, so I cannot test it, but maybe it could be as fast if with_aliases = bool(group._psygnal_aliases) is defined inside _setattr_and_emit_, before line 372. So it would simplify the code for maintenance.

I can commit the change I have in mind for the benchmark workflow to run if you want.

[tlambert03]

maybe it could be as fast if with_aliases = bool(group._psygnal_aliases) is defined inside setattr_and_emit, before line 372. So it would simplify the code for maintenance.

I agree! go for it

I don't know how to run the benchmarks in local,

the codspeed benchmarks actually can't be run in local, (only the asv ones, and this wasn't a regression in those benchmarks)... so i just guessed it would help and got lucky :)

I can commit the change I have in mind for the benchmark workflow to run if you want.

sure!

[getzze]

They can always use `group._psygnal_aliases` if they want. But I'm thinking now that it's probably useful to have an easy and user-friendly way to get the signal alias for a given field. Maybe we can have `get_signal_by_alias` be a function (that takes a `SignalGroup` as first arg) instead of a method?

…

On 21 March 2024 13:40:05 GMT+00:00, Talley Lambert ***@***.***> wrote: @tlambert03 commented on this pull request. > + def get_signal_by_alias(self, name: str) -> SignalInstance | None: + sig_name = self._psygnal_aliases.get(name, name) + if sig_name is None or sig_name not in self: + return None + return self[sig_name] i do think that end-users should also be able to search by alias. but i guess the question is should they no longer be able to search by the original name? i.e. if I remove the method on SignalGroup, then end-users can no longer access the original names for an aliased signal, correct? are we ok with that? -- Reply to this email directly or view it on GitHub: #299 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

[getzze]

maybe it could be as fast if with_aliases = bool(group._psygnal_aliases) is defined inside _setattr_and_emit_

Nope...

[tlambert03]

thanks @getzze. I'm still conflicted on how to present the fetching of signals by alias publicly. I don't like a top-level get_signal_from_field function. I removed and will just merge as is, without a public accessor. If you need one in the future, (i.e. outside of what the evented dataclass does for you already), let me know and we'll reconsider our options

[getzze]

It sounds good, thanks!