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

Update stix table with Unicode names #26830

Merged
merged 2 commits into from
Oct 12, 2023
Merged

Conversation

devRD
Copy link
Contributor

@devRD devRD commented Sep 19, 2023

Closes #25738
Closes #25990

PR summary

PR checklist

@devRD devRD marked this pull request as ready for review September 19, 2023 19:13
# Each element is a 4-tuple of the form:
# src_start, src_end, dst_font, dst_start
#
stix_virtual_fonts: dict[str, dict[str, list[tuple[int, int, str, int]]] |
Copy link
Member

Choose a reason for hiding this comment

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

You are losing the type hint from this line, which appears to be the cause of the mypy errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing it through the loop below for the ord check changes the type of the tuple to Tuple(str, str, str, str) which is different from the predefined Tuple(int, int, str, int) which causes mypy to throw an error. Any suggestion on how this can be handled better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Change the defined format to the new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a couple of issues with just updating the type of the dictionary. The new format for stix_virtual_fonts-- updated to Unicode names-- is in type tuple[str, str, str, (int | str)] which is what we want to change it to, but the dictionary used in StixFonts requires the type tuple[int, int, str, int] mapping. The type of value changes during the course and mypy seems to complain about that, because it wants to keep the type of the dictionary intact.

I guess the above issues get fixed with the new changes, but mypy still seems to complain about indexing and assignment type errors... 🤷

Copy link
Member

Choose a reason for hiding this comment

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

One option would be to use two different variables for each, though preferably one of them would be garbage collected pretty quick (maybe use del if it is top level, then)

Copy link
Contributor Author

@devRD devRD Sep 24, 2023

Choose a reason for hiding this comment

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

It looks like the assignment and indexing errors come from nesting the indexes like: stix_virtual_fonts[keys][i] where mypy seems to lose the type information and checks for str index in dict, but the stix_virtual_fonts[keys][i] type is a nested list with index: int.

This problem seems related to some of the open issues reported in mypy GitHub repo:
python/mypy#10146
python/mypy#13374
python/mypy#9176

Even with using two different variables, I think we would still need the nesting behavior to access/change the values(?)

A way would be to refactor the stix_virtual_fonts table to utilize mypy's TypedDict option, which requires a fixed schema to define the types, but that doesn't seem reasonable.

For lack of a better solution, I added the # type: ignore flag against the nested assignment and indexing, but that doesn't seem to work either... seems to work now.

@devRD devRD force-pushed the mt-fonts_table branch 9 times, most recently from f93b809 to 7f0a392 Compare September 25, 2023 09:50
@jklymak jklymak requested a review from QuLogic October 5, 2023 19:41
@ksunden
Copy link
Member

ksunden commented Oct 5, 2023

Here is what I was able to come up with to make it so that we don't have to type: ignore quite so much:

Essentially:

  • define some type aliases for repeated tuple definitions
  • initially define with strs, but then use a different variable defined as ints
  • recursive function that I think better shows what is being transformed
    • if tuple, transform the tuple
    • if list, iterate the list recursively
    • if dict, iterate the dict recursively, keeping the keys
  • Call the method instead of editing in place (and thereby changing types)
diff --git a/lib/matplotlib/_mathtext.py b/lib/matplotlib/_mathtext.py
index 0d2ca5a6d4..0d8842772a 100644
--- a/lib/matplotlib/_mathtext.py
+++ b/lib/matplotlib/_mathtext.py
@@ -812,15 +812,15 @@ class StixFonts(UnicodeFonts):
             while lo < hi:
                 mid = (lo+hi)//2
                 range = mapping[mid]
-                if uniindex < range[0]:  # type: ignore
+                if uniindex < range[0]:
                     hi = mid
-                elif uniindex <= range[1]:  # type: ignore
+                elif uniindex <= range[1]:
                     break
                 else:
                     lo = mid + 1
 
-            if range[0] <= uniindex <= range[1]:  # type: ignore
-                uniindex = uniindex - range[0] + range[3]  # type: ignore
+            if range[0] <= uniindex <= range[1]:
+                uniindex = uniindex - range[0] + range[3]
                 fontname = range[2]
             elif not doing_sans_conversion:
                 # This will generate a dummy character
diff --git a/lib/matplotlib/_mathtext_data.py b/lib/matplotlib/_mathtext_data.py
index c661f2031b..314b78f021 100644
--- a/lib/matplotlib/_mathtext_data.py
+++ b/lib/matplotlib/_mathtext_data.py
@@ -3,6 +3,7 @@ font data tables for truetype and afm computer modern fonts
 """
 
 from __future__ import annotations
+from typing import overload
 
 
 latex_to_bakoma = {
@@ -1113,10 +1114,11 @@ tex2uni = {
 # Each element is a 4-tuple of the form:
 #   src_start, src_end, dst_font, dst_start
 
-stix_virtual_fonts: dict[str, dict[str, list[tuple[
-    (str | int), (str | int), str, (str | int)]]] |
-                        list[tuple[
-                            (str | int), (str | int), str, (str | int)]]] = {
+_EntryTypeIn = tuple[str, str, str, str|int]
+_EntryTypeOut = tuple[int, int, str, int]
+
+_stix_virtual_fonts: dict[str, dict[str, list[_EntryTypeIn]] |
+                               list[_EntryTypeIn]] = {
     'bb':
         {
         "rm":
@@ -1718,24 +1720,29 @@ stix_virtual_fonts: dict[str, dict[str, list[tuple[
         ],
     }
 
-for keys in stix_virtual_fonts:
-    for i, k in enumerate(stix_virtual_fonts[keys]):
-        if isinstance(k, tuple):
-            # passes the value of tuple as is, if the index is '2'
-            # for the mentioned font types such as 'rm', 'it', 'bf'
-            # or when the tuple has a reserved hex value for the font.
-            # Otherwise it returns the int value of the Unicode name
+@overload
+def _normalize_stix_fontcodes(d: _EntryTypeIn) -> _EntryTypeOut: ...
+@overload
+def _normalize_stix_fontcodes(d: list[_EntryTypeIn]) -> list[_EntryTypeOut]: ...
+@overload
+def _normalize_stix_fontcodes(d: dict[str, list[_EntryTypeIn] |
+                                        dict[str, list[_EntryTypeIn]]]
+    ) -> dict[str, list[_EntryTypeOut] | dict[str, list[_EntryTypeOut]]]: ...
+
+def _normalize_stix_fontcodes(d):
+    if isinstance(d, tuple):
+        return tuple(ord(x) if isinstance(x, str) and len(x) == 1 else x for x in d)
+    elif isinstance(d, list):
+        return [_normalize_stix_fontcodes(x) for x in d]
+    elif isinstance(d, dict):
+        return {k: _normalize_stix_fontcodes(v) for k, v in d.items()}
+
+stix_virtual_fonts: dict[str, dict[str, list[_EntryTypeOut]] | list[_EntryTypeOut]]
+stix_virtual_fonts = _normalize_stix_fontcodes(_stix_virtual_fonts)
+
+# Free redundant list now that it has been normalized
+del _stix_virtual_fonts
 
-            stix_virtual_fonts[keys][i] = tuple( # type: ignore
-                    [x if idx == 2 or not isinstance(x, str)
-                     else int(ord(x)) for idx, x in
-                     enumerate(stix_virtual_fonts[keys][i])]) # type: ignore
-        else:
-            for m, mv in enumerate(stix_virtual_fonts[keys][k]): # type: ignore
-                stix_virtual_fonts[keys][k][m] = tuple(          # type: ignore
-                        [x if idx == 2 or not isinstance(x, str)
-                         else int(ord(x)) for idx, x in
-                         enumerate(stix_virtual_fonts[keys][k][m])]) # type: ignore
 
 # Fix some incorrect glyphs.
 stix_glyph_fixes = {

Co-authored-by: Kyle Sunden <git@ksunden.space>
@devRD
Copy link
Contributor Author

devRD commented Oct 9, 2023

I am not sure what went wrong with the mypy check here, but it ran without any mypy errors locally (?)

@ksunden
Copy link
Member

ksunden commented Oct 9, 2023

I think it is a python version thing, because the type aliases are evaluated at runtime, the str | int in the _EntryTypeIn definition needs to be replaced by Union[str, int]. This only affects 3.9, but our CI is running on 3.9. (Union needs to be imported from typing as well)

I think that should do it, and the number of errors is just snowballed from the fact that it doesn't see str | int as a valid type.

There are also some things that affect 3.8 in a similar vein, but we are now 3.9+ only, so not going to worry about those

Co-authored-by: Kyle Sunden <git@ksunden.space>
@ksunden ksunden merged commit 9bc8c53 into matplotlib:main Oct 12, 2023
40 checks passed
@QuLogic QuLogic added this to the v3.9.0 milestone Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MNT]: Improve readability of _mathtext_data.stix_virtual_fonts table
4 participants