search

parrt / tensor-sensor

The goal of this library is to generate more helpful exception messages for matrix algebra expressions for numpy, pytorch, jax, tensorflow, keras, fastai.
https://github.com/parrt/tensor-sensor
MIT License
794 stars 40 forks source link

Default font's spacing on mac incorrect #34

Closed sbrugman closed 3 years ago

sbrugman commented 3 years ago

Default font

Compare with courier new

Courier New

Relevant code:

with ts.explain(fontname="Courier New"):
    y = np.multiply.outer(x, np.arange(2.0))
parrt commented 3 years ago

Weird. is Consolas font installed on your system? Is it Intel or M1?

sbrugman commented 3 years ago

Consolas wasn't installed in my environment (Intel). The fallback font is "Dejavu Sans", which fails to display the code as seen above.

Advising users to install the font may involve recreating the environment etc. An alternative solution to consider is to explicitly specify a fallback font. This is much more user friendly in my opinion at nearly no burden on the package. For instance, "DejaVu Sans Mono" works well. This can be trivially achieved providing a sequence of font names:

parrt commented 3 years ago

agreed this is a good change. I guess your PR only sets the arg default, but there's no code to fall back to the indicated font. Font name is now a tuple by default, which is really a comment for the developer. Shouldn't there be logic to check for font installations and use one that works?

sbrugman commented 3 years ago

Matplotlib handles all that logic, this PR just changes the default:

WARNING:matplotlib.font_manager:findfont: Font family ('Consolas', 'Consolas Sans Mono') not found. Falling back to DejaVu Sans.

parrt commented 3 years ago

You are kidding me!? Wow. that's insane. You just took me to school!

parrt commented 3 years ago

By the way gotta use "Fixes #34" not "Fix for #34" to get autoclose. :)