Replace invalid symbols in the labels for metadata visualization #1670
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1670 +/- ##
==========================================
- Coverage 96.97% 96.95% -0.03%
==========================================
Files 47 47
Lines 4263 4265 +2
==========================================
+ Hits 4134 4135 +1
- Misses 129 130 +1
☔ View full report in Codecov by Sentry. |
fealho
requested review from
R-Palazzo and
amontanez24
and removed request for
a team
| @@ -105,10 +107,10 @@ def visualize_graph(nodes, edges, filepath=None): | |||
| ) | |||
|
|
|||
| for name, label in nodes.items(): | |||
| digraph.node(name, label=graphviz.escape(label)) | |||
| digraph.node(name, label=_replace_special_characters(label)) | |||
There was a problem hiding this comment.
have you tried using https://graphviz.readthedocs.io/en/stable/api.html#graphviz.nohtml
There was a problem hiding this comment.
Yes, it doesn't address the problem.
There was a problem hiding this comment.
I see. It seems like it works if you replace '>' with '\>'. Then in the output graph the label looks correct
There was a problem hiding this comment.
Just a note, this approach removes the backslash from the column name. So if a column has \< in it, the output will be <.
There was a problem hiding this comment.
I think that's ok since prior to this change if they had '\>' in a label, it would only show '>' anyway.
There was a problem hiding this comment.
About this, can we add an integration test where we fit and sample a synthesizer after the visualize() to ensure we're not breaking the metadata validation or the fit and sample with the change
| @@ -105,10 +107,10 @@ def visualize_graph(nodes, edges, filepath=None): | |||
| ) | |||
|
|
|||
| for name, label in nodes.items(): | |||
| digraph.node(name, label=graphviz.escape(label)) | |||
| digraph.node(name, label=_replace_special_characters(label)) | |||
There was a problem hiding this comment.
I think that's ok since prior to this change if they had '\>' in a label, it would only show '>' anyway.
| data1 = pd.DataFrame({'>': ['a', 'b', 'c']}) | ||
| data2 = pd.DataFrame({'>': ['a', 'b', 'c']}) |
There was a problem hiding this comment.
Can we test both symbols? and maybe the symbols mixed with text
|
|
||
|
|
||
| def test_visualize_graph_for_multi_table(): | ||
| """Test it runs when a column name contains symbols.""" |
There was a problem hiding this comment.
You can put a different message that the test before to be more specific
CU-86ayfuk2q
Resolve #1625.