feat: add signal aliases on SignalGroup

[getzze]

Split from #265 solves https://github.com/pyapp-kit/psygnal/issues/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[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[codspeed-hq[bot]]

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 was able to avoid the slight performance regression with https://github.com/pyapp-kit/psygnal/pull/299/commits/64e2fb4740816af427cc0408716e72f58387a556

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_andemit, 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: https://github.com/pyapp-kit/psygnal/pull/299#discussion_r1533931738 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!