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

epic: clean up nanoplot implementation #420

Open
machow opened this issue Aug 19, 2024 · 1 comment
Open

epic: clean up nanoplot implementation #420

machow opened this issue Aug 19, 2024 · 1 comment
Assignees
Labels
.epic Big features and initiatives

Comments

@machow
Copy link
Collaborator

machow commented Aug 19, 2024

From pairing, we currently have pieces like boxplots in nanoplots teed up (#168). However, when we merged the nanoplot PR, we decided to punt on doing too much review / cleanup, in the name of getting it out.

Let's plan on cleaning up the nanoplot implementation a bit, so it's simpler to add things like boxplots. Below are some potential cleanups.

Avoiding unsetting arguments based on other arguments

We decide what goes into _construct_nanoplot_svg(), but currently it can quietly unset arguments.

ref_area_tags = "" if ref_area_tags is None or show_reference_area is False else ref_area_tags
area_path_tags = "" if area_path_tags is None or show_data_area is False else area_path_tags
data_path_tags = "" if data_path_tags is None or show_data_line is False else data_path_tags
zero_line_tags = "" if zero_line_tags is None else zero_line_tags
bar_tags = "" if bar_tags is None else bar_tags
ref_line_tags = "" if ref_line_tags is None or show_reference_line is False else ref_line_tags
circle_tags = "" if circle_tags is None or show_data_points is False else circle_tags
g_y_axis_tags = "" if g_y_axis_tags is None or show_y_axis_guide is False else g_y_axis_tags
g_guide_tags = "" if g_guide_tags is None or show_vertical_guides is False else g_guide_tags

Do not fallback to unspecified behavior

For example, this if/else clause defaults to using a specific calculation (quartile 3).

else:
ref_line = _gt_q3(vals)

(Note that functions outside this one check that a calculation was specified, so in practice things work out okay)

Consolidate large branches inside _generate_nanoplot()

For example, the "Generate curved data line section" has two branches that seem similar to each other.

#
# Generate a curved data line
#
if plot_type == "line" and show_data_line and data_line_type == "curved":

It seems like if we had functions for curved_path() and polyline() that produced svg strings (or a data repr of SVG elements), that would help a lot. IMO everywhere that the variable data_path_tags gets used gives helpful hints for consolidating.

People can go overboard consolidating, so I think the key is finding the places where it cleans up readability / robustness the most.

@machow machow added the .epic Big features and initiatives label Aug 22, 2024
@rich-iannone
Copy link
Member

In speaking with @t-kalinowski we can do the following to improve this part of the codebase:

  1. split _generate_nanoplot() into several smaller functions; try to reduce line length to ~20 lines
  2. create a nanoplot spec which encapsulates all data and options in a class that initializes the values from input data and collected args
  3. ensure there are reasonable contracts for each of the methods added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.epic Big features and initiatives
Projects
None yet
Development

No branches or pull requests

2 participants