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

"New engine" changed behavior #155

Closed
JAGulin opened this issue Oct 5, 2023 · 4 comments
Closed

"New engine" changed behavior #155

JAGulin opened this issue Oct 5, 2023 · 4 comments

Comments

@JAGulin
Copy link

JAGulin commented Oct 5, 2023

After the b98938 "Implement new engine!" commit, the examples are subjectively worse than before.
https://github.com/Phlya/adjustText/blob/b98938fcb06e9c27772c50074f601a45626e8d2a/docs/source/Examples.ipynb

Not sure if the commited examples are generated from the exact code, but it's similar to the "latest" when I tested locally and the latest RTD doc.
https://adjusttext.readthedocs.io/en/latest/Examples.html

I consider the collected examples the best testcases we have, so my question is if this is a known degrade, if further investigation is needed or if the examples should be reworked to fit the new engine.

The 32f0335 from ver 8.0 looks better:
https://github.com/Phlya/adjustText/blob/32f0335375c0e0586a5ee49ee458e42ffc2320fa/docs/source/Examples.ipynb

My subjective observations:

  • The return value from adjust_text has been removed. I guess it was by design, any good reason? Maybe provide some other way to get debugging/performance values out, if you didn't like the return value.
  • Would it be bad to offer both time and count limit?
  • Second image: Text no longer avoids the dots, possibly because the dots are too big, or the gravity is not intended.
  • Third image: Few arrows and none seem to have the arrow head.
  • Fifth "something": Clearance could be better, but the arrow is definitely missing.
  • 8th "eucs": No arrow head, but otherwise this is actually an improvement from before.
  • page-impressions: Slightly better when I ran locally, a risk with "time based limit" is that behavior/result may different based on machine specs.
  • page-impressions: Actually two plots where both use the same adjustment. Meant to remove one or an illustration of something?
  • langnameGroup: Lots of overlap, but hardly worse than before. x-direction is very good still.
  • coastlines: Readability is not much worse than before, but almost no arrows. How clear is 9 15 19?

If you confirm the intention, I may be able to help sorting it out.

@Phlya
Copy link
Owner

Phlya commented Oct 6, 2023

Thank you for the feedback. From a quick look, I think this is a combination of things... First, there is the new "pull-back" step, which forces the labels to be much closer to the points that they correspond to, and helps avoid cases when they fly away like crazy. You can set it to 0 with force_pull=(0, 0). Second, there is a new argument min_arrow_len=5, which makes it not to add an arrow when the label is too close to the point. I think super short arrows don't look tidy and in most cases aren't necessary.
An additional issue is I find the display of arrows is just somehow sometimes buggy in matplotlib, and that's another reason I try not to add them when not necessary... #79 matplotlib/matplotlib#6162

I agree re the examples being the best (and only) result quality tests we (can) have, so it's great that you did this comparison, thank you again!

If you want to look into it, you could try to find different (default?) arguments that you think would work better, and hopefully there is no need to go deeper than that. Please let me know what you think.

@JAGulin
Copy link
Author

JAGulin commented Oct 8, 2023

Thanks for quick reply. Being a (planned) major version change, I suppose backward compatibility is not mandatory, and the ultimate goal for any version is rather to have perfect render out of the box (mostly default parameters). "Perfect" is likely hard to reach, so a major decision is how to balance opinions. :-)

examples being the best (and only) result quality tests we (can) have

What example did you focus on when you made the new engine? Any new tests I should keep in mind?

Not sure if there are "old users" to cater for or if the package is mostly used for one-time graphs. And of course, there's no mandatory reason to upgrade for anyone happy with v0.8 or so. But the set of examples is an indication that keeping the "old style" is useful.

Also, seeing that plotnine seems to use alignText internally, they would probably be happy for heads up before major changes.
https://github.com/has2k1/plotnine/blob/main/pyproject.toml#L46

you could try to find different (default?) arguments that you think would work better,

If "new style" is objectively better, that's a win for everyone. My worry is that I don't know how deterministic the layout is. It would be sad if I fine-tune defaults on my system or on the documentation server, but this is not what "real users" get.

I haven't looked at the algorithm yet, so I may not be the best for the task, but I think it's good to set some common expectations:
What version of matplot do we target, wrapper libs like plotnine, etc. Will screen size, CPU speed or anything else affect the results?
From issues I've see DPI, z-order and engine used for matplot is added to the list of things to keep in control.

I think super short arrows don't look tidy and in most cases aren't necessary.

I agree may sometimes be omitted, but I think the criteria for "needed" is not length, but ambiguity.
https://adjusttext.readthedocs.io/en/latest/_images/Examples_10_0.png
For me, this can't work without arrow. Possibly this plot is not a good design at all, but "invisible text anchor" will hardly be able to showcase the right point. I can try min_arrow_len=0 also, to see if that get's closer to "old style".

there is the new "pull-back" step, which forces the labels to be much closer to the points

That's not a bad idea, if it can be properly balanced. Clearance should be top prio, I think.
One thing I noticed with the examples is that they have fairly large "markers". I suppose the "point" that adjustText avoids is much smaller. It will be hard to avoid large markers if they aren't added as objects, right?

and hopefully there is no need to go deeper than that.

Do you have any additional code for debugging? I did have some thoughts about the changed behavior:

The return value from adjust_text has been removed. I guess it was by design, any good reason? Maybe provide some other way to get debugging/performance values out, if you didn't like the return value.
Would it be bad to offer both time and count limit?

I saw some reasearch papers mentioned. Do you keep track of any other implementations, like the ggrepel?

@Phlya
Copy link
Owner

Phlya commented Mar 18, 2024

I think I have fixed some bugs introduced in the "new engine", and it should work better now!

@Phlya
Copy link
Owner

Phlya commented Mar 18, 2024

FYI looking at the examples, basically default parameters work well almost everywhere.
Of note, the result is typically better when texts are drawn on top of their target points, so with ha='center', va='center'.

I sometimes glance at the ggrepel repo, but don't really keep track of papers...

Let me know if you have any more important questions (feel free to reopen!), and sorry it took a while to fix things :) Thanks for a detailed report and assessment.

@Phlya Phlya closed this as completed Mar 18, 2024
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

No branches or pull requests

2 participants