Skip to content

Support unwrapping complex phase values#1947

Merged
axelboc merged 1 commit into
mainfrom
unwrap-phase
Jan 16, 2026
Merged

Support unwrapping complex phase values#1947
axelboc merged 1 commit into
mainfrom
unwrap-phase

Conversation

@axelboc

@axelboc axelboc commented Jan 16, 2026

Copy link
Copy Markdown
Contributor

Fix #1925

I add an option called "Phase (unwrapped)" to the complex vis type selector in the toolbar of the complex line visusalisation:

Screencast.from.2026-01-16.13-06-22.webm

In the proposed implementation, when selecting the unwrapped option in the drop-down, the phase/amplitude values are recomputed, which kind of breaks the optimisation I had of precomputing the arrays on initial render. The alternative would be for usePhaseAmplitudeArrays to return three arrays (of arrays because of auxiliary signals): phaseArrays, unwrappedPhaseArrays and amplitudeArrays, but this felt messier.

I'm tempted to go back to having no optimisation at all and to just compute the phase/amplitude values when changing the complex vis type... Not sure...

Comment thread packages/app/src/vis-packs/core/complex/hooks.ts
Comment thread packages/app/src/vis-packs/core/complex/MappedComplexLineVis.tsx Outdated
Comment thread packages/app/src/vis-packs/core/heatmap/HeatmapToolbar.tsx
Comment thread packages/shared/src/mock-values.ts
@axelboc axelboc requested a review from loichuder January 16, 2026 12:20

@loichuder loichuder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The alternative would be for usePhaseAmplitudeArrays to return three arrays (of arrays because of auxiliary signals): phaseArrays, unwrappedPhaseArrays and amplitudeArrays, but this felt messier.

I'm tempted to go back to having no optimisation at all and to just compute the phase/amplitude values when changing the complex vis type... Not sure...

Yeah it feels like we are in some kind of in-between here. Personally, I would have gone for the three arrays returned by usePhaseAmplitudeArrays. What exactly felt messy in this case ?

Don't get me wrong, I am still happy with the PR and have no qualms merging like this.

@NAThompson

NAThompson commented Jan 16, 2026

Copy link
Copy Markdown

I'm tempted to go back to having no optimisation at all and to just compute the phase/amplitude values when changing the complex vis type

The unwrap computation is super cheap relative to rendering right? I might just recompute on the fly.

@axelboc

axelboc commented Jan 16, 2026

Copy link
Copy Markdown
Contributor Author

@NAThompson we try not to loop over the values after toolbar interactions, as it can make the UI feel laggy for large arrays. Of course, at some point, as you've experienced, interacting with the visualization itself becomes laggy, but before we get to this extreme case, extra loops can really slow things down.

It's not as bad as I first thought with the three arrays returned from usePhaseAmplitudeArrays. It's just not very scalable — I hope we don't need to add more complex vis type options in the future... 🤣

@loichuder

Copy link
Copy Markdown
Member

It's not as bad as I first thought with the three arrays returned from usePhaseAmplitudeArrays. It's just not very scalable

Yeah, for sure. But I hope another complex vis option will not be needed for a while

@loichuder loichuder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Me likes it 👍

@axelboc axelboc merged commit febde4f into main Jan 16, 2026
13 checks passed
@axelboc axelboc deleted the unwrap-phase branch January 16, 2026 15:24
@NAThompson

Copy link
Copy Markdown

@axelboc : Thanks for all your hard work! It's awesome to see this get merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Phase should(?) be unwrapped in complex number visualization

3 participants