Skip to content

Conversation

@shroffk
Copy link
Member

@shroffk shroffk commented Oct 3, 2025

Addresses #3577

Includes basic unit tests
Test are passing

@shroffk
Copy link
Member Author

shroffk commented Oct 3, 2025

Added the missing documentation.

@shroffk shroffk requested a review from jacomago October 3, 2025 18:27
@shroffk
Copy link
Member Author

shroffk commented Oct 3, 2025

20251003-1916-56 6173498

downsampled from 1000 to 100 points

@georgweiss
Copy link
Collaborator

This is cool!
Guess the purpose of the formula function is to off-load plot rendering code, even though raw data must be processed anyway.

Copy link
Collaborator

@georgweiss georgweiss left a comment

Choose a reason for hiding this comment

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

Since by default function arguments are initialized as VDouble in FormulaPV L66, ArraySampleWithLTTBFunction#compute() will throw an exception. For the same case ArrayScalarDivisionFunction returns DEFAULT_NAN_DOUBLE_ARRAY. Should this be aligned?

@shroffk
Copy link
Member Author

shroffk commented Oct 6, 2025

Since by default function arguments are initialized as VDouble in FormulaPV L66, ArraySampleWithLTTBFunction#compute() will throw an exception. For the same case ArrayScalarDivisionFunction returns DEFAULT_NAN_DOUBLE_ARRAY. Should this be aligned?

hmm... you are right, we should align this behaviour

In some of the array functions we are throwing exceptions if an expected array argument is not one... but like you mention the initialization issue means we get at least one exception

If we just return an nan array then how do we convey that the input is not of the correct type?

@georgweiss
Copy link
Collaborator

You're right too, we need to highlight wrong type... But can we do it less verbose, the stack trace caught my eye which made me investigate it.

@shroffk
Copy link
Member Author

shroffk commented Oct 6, 2025

A single line log message? a warning

@georgweiss
Copy link
Collaborator

Personally I'd prefer single line, either warning or severe

@georgweiss
Copy link
Collaborator

Maybe a warning including "....maybe due to mismatch in initial type" or similar

@shroffk shroffk merged commit 189c8bc into master Oct 7, 2025
3 checks passed
@shroffk shroffk deleted the LTTB branch October 21, 2025 20:42
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.

3 participants