Persistence graphical tools improvements #504

VincentRouvreau · 2021-06-17T13:46:08Z

Fix #453
Fix #461
Review error management (no more prints, warnings and exceptions instead) and add unitary tests
No more plots in loop (optimization)

…ots in plot_persistence_diagram. Fix GUDHI#453 and factorize code

…e same dimensions as C as it is deprecated since 3.3

…d exceptions instead)

Hind-M · 2021-06-23T09:11:42Z

src/python/gudhi/persistence_graphical_tools.py

-    '''
-    if isinstance(a[0][1], np.float64) or isinstance(a[0][1], float):
+    """
+    if isinstance(a[0][1], np.floating) or isinstance(a[0][1], float):


Do we need to check both np.floating and float ? I'm wondering if np.floating already includes float since it handles all floating-point scalar types.

np.floating includes np.float64 and np.float32.
I don't know if it can happen, but I find nice we can plot float64 and float32.

Ah okay, the type to check is different even if it could be the same precision.

np.issubdtype(type(a[0][1]), np.floating)?

(for the syntax, isinstance(x,t1) or isinstance(x,t2) can be written isinstance(x,(t1,t2)) I believe)

Ok thanks, I didn't know this nicer syntax. Done on 87da488

mglisse · 2021-08-12T14:19:55Z

The comment "fix #453" is a bit short, and the changes are a bit hard to find with the reformatting.
What is the new behavior? By default it prints a warning once, the first time the plot function is called with a list that is too long?

VincentRouvreau · 2021-08-24T13:18:39Z

The comment "fix #453" is a bit short, and the changes are a bit hard to find with the reformatting.
What is the new behavior? By default it prints a warning once, the first time the plot function is called with a list that is too long?

I let the warning filter to default (which means "print the first occurrence of matching warnings for each location (module + line number) where the warning is issued").

I could set it to "always", but maybe we should define a class like a PlotWarning for instance to not impact other warnings.

Or maybe I should just document the behaviour.

Hind-M · 2021-09-08T08:06:49Z

src/python/gudhi/persistence_graphical_tools.py

 @lru_cache(maxsize=1)
 def _matplotlib_can_use_tex():
    """This function returns True if matplotlib can deal with LaTeX, False otherwise.
    The returned value is cached.
    """
    try:
        from matplotlib import checkdep_usetex
+
        return checkdep_usetex(True)
    except ImportError:


Wouldn't it be more explicit if we print the import error instead?
For example:

Suggested change

except ImportError:

except ImportError as error:

Done on 55b15a7

Hind-M · 2021-09-08T08:07:42Z

src/python/gudhi/persistence_graphical_tools.py

        return checkdep_usetex(True)
    except ImportError:
-        print("This function is not available, you may be missing matplotlib.")
+        warnings.warn("This function is not available, you may be missing matplotlib.")


Suggested change

warnings.warn("This function is not available, you may be missing matplotlib.")

print(error)

I kept the use of warning and modify the message on 55b15a7
error is not directly printable, dev only have access to its name and path (cf. ImportError)

Hind-M · 2021-09-08T08:11:34Z

src/python/gudhi/persistence_graphical_tools.py

        return axes

    except ImportError:
-        print("This function is not available, you may be missing matplotlib.")
+        warnings.warn("This function is not available, you may be missing matplotlib.")


Same here with the import error print ?

Hind-M · 2021-09-08T08:12:32Z

src/python/gudhi/persistence_graphical_tools.py

        return axes

    except ImportError:
-        print("This function is not available, you may be missing matplotlib.")
+        warnings.warn("This function is not available, you may be missing matplotlib.")


Same here with the import error print ?

Hind-M · 2021-09-08T08:13:06Z

src/python/gudhi/persistence_graphical_tools.py

-        print(
-            "This function is not available, you may be missing matplotlib and/or scipy."
-        )
+        warnings.warn("This function is not available, you may be missing matplotlib and/or scipy.")


Same here with the import error print ?

…ical_tools_improvements

mglisse

I didn't really check the details, but if you think it is good, go ahead and merge it.

mglisse · 2022-04-29T18:48:45Z

src/python/gudhi/persistence_graphical_tools.py

+    except ImportError as import_error:
+        warnings.warn(f"This function is not available.\nModuleNotFoundError: No module named '{import_error.name}'.")


I don't really see how that's better than not catching the exception, but it was already like that before the PR, so ok.

VincentRouvreau added 6 commits June 15, 2021 18:01

Remove deprecated max_barcodes in plot_persistence_barcode and max_pl…

17da284

…ots in plot_persistence_diagram. Fix GUDHI#453 and factorize code

Fix MatplotlibDeprecationWarning: shading='flat' when X and Y have th…

8587b70

…e same dimensions as C as it is deprecated since 3.3

Ignore figure as not used on default subplot

2eb2f96

Use float as it can be ambiguous. Do not work if we remove np.inf value

486b281

Fix GUDHI#461 and review all error cases (no more prints, warnings an…

cb10892

…d exceptions instead)

Forgot to add test file for graphical

72f7277

VincentRouvreau marked this pull request as ready for review June 21, 2021 12:34

black -l 120 modified files

f9b1e50

Hind-M reviewed Jun 23, 2021

View reviewed changes

Hind-M approved these changes Jun 24, 2021

View reviewed changes

VincentRouvreau mentioned this pull request Aug 2, 2021

Windows CI failure #513

Closed

code review: isinstance can take several instances types as a list

87da488

Hind-M reviewed Sep 8, 2021

View reviewed changes

VincentRouvreau added 3 commits April 6, 2022 09:40

Merge remote-tracking branch 'upstream/master' into persistence_graph…

e957f64

…ical_tools_improvements

code review: Rewrite warnings in case of import errors

55b15a7

plot_persistence_diagram and plot_persistence_barcode improvements

79278cf

VincentRouvreau added the optimization It works, now make it faster label Apr 29, 2022

mglisse approved these changes Apr 29, 2022

View reviewed changes

VincentRouvreau merged commit 7491d4b into GUDHI:master May 9, 2022

VincentRouvreau deleted the persistence_graphical_tools_improvements branch May 9, 2022 20:12

VincentRouvreau added the 3.6.0 GUDHI version 3.6.0 label May 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persistence graphical tools improvements #504

Persistence graphical tools improvements #504

VincentRouvreau commented Jun 17, 2021 •

edited

Loading

Hind-M Jun 23, 2021

VincentRouvreau Jun 24, 2021

Hind-M Jun 24, 2021

mglisse Jun 24, 2021

mglisse Jun 24, 2021

VincentRouvreau Aug 24, 2021 •

edited

Loading

mglisse commented Aug 12, 2021

VincentRouvreau commented Aug 24, 2021 •

edited

Loading

Hind-M Sep 8, 2021 •

edited

Loading

VincentRouvreau Apr 6, 2022

Hind-M Sep 8, 2021

VincentRouvreau Apr 6, 2022

Hind-M Sep 8, 2021

VincentRouvreau Apr 6, 2022

Hind-M Sep 8, 2021

VincentRouvreau Apr 6, 2022

Hind-M Sep 8, 2021

VincentRouvreau Apr 6, 2022

mglisse left a comment

mglisse Apr 29, 2022

	warnings.warn("This function is not available, you may be missing matplotlib.")
	print(error)

		except ImportError as import_error:
		warnings.warn(f"This function is not available.\nModuleNotFoundError: No module named '{import_error.name}'.")

Persistence graphical tools improvements #504

Persistence graphical tools improvements #504

Conversation

VincentRouvreau commented Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

mglisse commented Aug 12, 2021

VincentRouvreau commented Aug 24, 2021 • edited Loading

Hind-M Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mglisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau commented Jun 17, 2021 •

edited

Loading

VincentRouvreau Aug 24, 2021 •

edited

Loading

VincentRouvreau commented Aug 24, 2021 •

edited

Loading

Hind-M Sep 8, 2021 •

edited

Loading