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.

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

Rewrite _preprocess for performance and customization (BREAKING) #55

Merged
merged 8 commits into from
Aug 7, 2024

Conversation

disberd
Copy link
Member

@disberd disberd commented Aug 7, 2024

This PR fully rewrite the internal way _preprocess works, making it more performant and more customizable.

This is also now split into two different function:

  • _process_with_names: This is the new function to call for processing and accepts multiple arguments as specified below
  • _preprocess: This is the old function only expecting a single argument as input and doing transformation on it. By default, the generic fallback of _process_with_names calls this

The standard signature for a _process_with_names method is:

_process_with_names(x, fl::Val, @nospecialize(args::Vararg{AttrName}))

where

  • the first argument should be the actual input to process and should be typed accordingly for dispatch.
  • The second argument is either Val{true} or Val{false} and represents the flag to force number to be converted in Float32.
    • We added this to significantly improve performance as the runtime check for converting or not was creating type instability.
  • All the remaining arguments are of type AttrName and represent the path of attributes names leading to this specific input. For example, if we are processing the input that is inside the xaxis_range in the layout, the function
    call will have this form:
    • _process_with_names(x, fl, AttrName(:xaxis), AttrName(:range), AttrName(:layout))

This again is to allow dispatch to work on the path so that one can customize behavior of _process_with_names with great control.
At the moment this is only used for modifying the behavior when title is passed as a String, changing it to the more recent plotly syntax (closing #51)

As part of this PR we also removed the skip_float32 function and renamed the ScopedValue to FORCE_FLOAT32 (which is now true by default).
This is directly checked only in the method _process_with_names(p::PlutoPlot)

@disberd disberd added the BREAKING Breaking change label Aug 7, 2024
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.47%. Comparing base (73b670d) to head (f5be86b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
+ Coverage   89.89%   90.47%   +0.58%     
==========================================
  Files          12       12              
  Lines         287      294       +7     
==========================================
+ Hits          258      266       +8     
+ Misses         29       28       -1     

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

@disberd disberd merged commit 4884a4e into main Aug 7, 2024
8 checks passed
@disberd disberd deleted the path_in_preprocess branch August 7, 2024 11:09
@disberd disberd changed the title Rewrite _preprocess for more performance and customization Rewrite _preprocess for performance and customization (BREAKING) Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

title provided as string should be transformed into attr(;text = <provided title>)
1 participant