Skip to content

Tweak aesthetics of RD-plot; fix SZ3 as normalizer#31

Merged
treigerm merged 5 commits into
mainfrom
tweak-plots
Jun 2, 2025
Merged

Tweak aesthetics of RD-plot; fix SZ3 as normalizer#31
treigerm merged 5 commits into
mainfrom
tweak-plots

Conversation

@treigerm
Copy link
Copy Markdown
Member

This PR does three things:

  • Tweaks the aesthetics of the plot used in the presentation (someone gave feedback that the "Better" arrow wasn't as visible as it could be)
  • Fixes the normalizer to SZ3. With the updated JPEG2000 quantization process, JPEG2000 now gives higher compression ratios but also larger errors and always exceeds the error bound. So using it as a normalizer isn't great for the figure aesthetics because most compressor results will be squished on one side of the plot. So in the code I know set SZ3 consistently as the way to normalize.
  • I had to adjust the way that the PDFs get saved. The interface with explicitly opening the file with wb was leading to corrupted PDFs (I couldn't really figure out why, saving PNGs works fine)

title_fontsize=14,
)
plt.xlabel(
r"Median Compression Ratio Relative to SZ3 ($\uparrow$)", fontsize=16
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should the labels be updated depending on what we normalize by?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The normalizer is now fixed to be SZ3 for the reason outlined above, so no need to update the label!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it really fixed now since above it looks like the normalizer can be configured?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's fixed here:

normalized_df = normalize(df, bound_normalize="mid", normalizer="sz3")

I still let the normalize function be more flexible because we might want to change the normalizer in the future.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

My issues is that the plot_aggregated_rd_curve function is public and so cannot assume that it is only called in this one place where SZ3 is fixed. It should either be private (though I'd like to keep it exposed) or also take a normalizer name parameter

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah good point, changed it now! @juntyr

if ispdf:
# Saving a PDF with the alternative code below leads to a corrupted file.
# Hence, we use the default savefig method.
plt.savefig(outfile, dpi=300)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

At least we should add a warning comment here saying that virtual UPath's are only support with the second method

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a comment!

@juntyr
Copy link
Copy Markdown
Collaborator

juntyr commented Apr 25, 2025

@treigerm Could you show a before and after of the RD-plot?

@treigerm
Copy link
Copy Markdown
Member Author

Apologies for the delay on getting back to this PR!

This is before the changes:
Screenshot 2025-05-21 at 11 32 29
and after:
Screenshot 2025-05-21 at 11 34 37

@juntyr
Copy link
Copy Markdown
Collaborator

juntyr commented May 28, 2025

Could you open an issue about the PDF corruption, I'd like to track the conflict between supporting virtual paths in the online lab and producing PDFs so that we can resolve it later

@treigerm
Copy link
Copy Markdown
Member Author

Done here #40 . Are you okay with me merging this @juntyr ?

@treigerm treigerm merged commit d3c726c into main Jun 2, 2025
3 checks passed
@treigerm treigerm deleted the tweak-plots branch July 25, 2025 11:24
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.

2 participants