Fix latex repr of symbolic distributions #6231

mattiadg · 2022-10-19T22:33:00Z

Do not put \operatorname inside \text
add test

What is this PR about?
When a symbolic distribution is defined as parameter of another one, the latex representation is broken.
This PR aims to solve this.

I think that the new failing tests are testing the wrong behavior, this is a draft because I would like someone more experienced with this to check on them.
What I'm doing is to remove \operatorname from inside \text because it's wrong.

Checklist

Explain important implementation details 👆
Make sure that the pre-commit linting/style checks pass.
Link relevant issues (preferably in nice commit messages)
Are the changes covered by tests and docstrings?
Fill out the short summary sections 👇

Major / Breaking Changes

...

Bugfixes / New features

Fix broken latex representation of distributions inside other distributions

Docs / Maintenance

...

* Do not put \operatorname inside \text * add test

codecov · 2022-10-20T10:32:27Z

Codecov Report

Merging #6231 (1d5940d) into main (9105d74) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6231   +/-   ##
=======================================
  Coverage   94.14%   94.14%           
=======================================
  Files         100      100           
  Lines       21235    21239    +4     
=======================================
+ Hits        19992    19996    +4     
  Misses       1243     1243

Impacted Files	Coverage Δ
pymc/printing.py	`86.55% <100.00%> (+0.46%)`	⬆️

…-devs#6232) * Adding option to include transformed variables in InferenceData Co-authored-by: Michael Osthege <[email protected]>

* remove them from the string borders since they always represent the math environment

* remove them from the string borders since they always represent the math environment * fix tests to check the correct behavior

# Conflicts: # pymc/tests/test_printing.py

mattiadg · 2022-10-24T22:07:23Z

Is anything else needed here?

# Conflicts: # pymc/tests/test_printing.py

mattiadg · 2022-10-25T07:06:24Z

I've already solved conflicts twice because people keep adding wrong tests for the latex printing.
That functionality is currently broken as #6202 shows, so the tests must be wrong in order to fail.
Please, have a loot at this PR before merging additional code in test_printing.py.

ricardoV94 · 2022-10-25T08:21:51Z

I've already solved conflicts twice because people keep adding wrong tests for the latex printing. That functionality is currently broken as #6202 shows, so the tests must be wrong in order to fail. Please, have a loot at this PR before merging additional code in test_printing.py.

That sometimes happens, a reviewer may be able to get to another PR and merge it before yours. Sorry for the trouble, but it's also something one must get used to :)

mattiadg · 2022-10-25T08:53:46Z

Yes, I understand that, I don't want to look upset. I'm just saying that the current tests in that file, and the ones are being added to it, are (partially) testing the wrong behavior, which is the reason why that issue exists in the first place.

Spaak

LGTM in general, though the testing should be fixed.

pymc/printing.py

…atex command is used

Spaak

LGTM!

Spaak · 2022-10-27T07:15:28Z

Looks good to me, thanks for adding the tests so quickly. I'll leave merging to someone else (eg @ricardoV94) because it's been a while for me.

mattiadg · 2022-10-27T07:38:23Z

Looks good to me, thanks for adding the tests so quickly. I'll leave merging to someone else (eg @ricardoV94) because it's been a while for me.

That was easy, I added as tests the exact same distributions used in the issue to show the problem. This way it's also possible to reference back to them, should something go wrong in the future.
However, I still need to understand the two branches of Codecov: patch and project. I am still not sure why patch became lower initially

pymc/tests/test_printing.py

larryshamalama

Looks good, thanks for your help! Glad that you were able to solve the other conflicts following the merge of #6242

larryshamalama · 2022-10-27T12:26:15Z

pymc/printing.py

+        return r"f(" + ", ".join([n.strip("$") for n in names]) + ")"
+
+
+def _latex_text_format(text: str) -> str:


Okay for now, but I have a sense that this can be simplified since you're always using it on an output of _latex_escape

I can add the call to _latex_escape(text.strip("$") inside _latex_text_format and call this also in the other 2 occurrences of _latex_escape. Then, we can also remove the naked \text{ in there.

michaelosthege · 2022-10-30T09:58:43Z

@mattiadg there appears to be one more conflict?

Would be great to push this over the finish line as we have a release coming up!

mattiadg · 2022-10-30T14:15:25Z

Sure, I will do it this evening or tomorrow

# Conflicts: # pymc/tests/test_printing.py

mattiadg · 2022-10-30T21:04:30Z

@mattiadg there appears to be one more conflict?

Would be great to push this over the finish line as we have a release coming up!

done

michaelosthege · 2022-10-30T21:59:35Z

Nice, thank you for fixing these @mattiadg !

Fix latex repr of nested variables

deb7764

* Do not put \operatorname inside \text * add test

ricardoV94 marked this pull request as draft October 20, 2022 10:08

ricardoV94 changed the title ~~[DRAFT] Fix latex repr of symbolic distributions~~ Fix latex repr of symbolic distributions Oct 20, 2022

dfm and others added 5 commits October 21, 2022 09:19

Adding option to include transformed variables in InferenceData (pymc…

8a07528

…-devs#6232) * Adding option to include transformed variables in InferenceData Co-authored-by: Michael Osthege <[email protected]>

Time series clean up (pymc-devs#6240)

005818b

Remove inner $ symbols from latex representations

e82404f

* remove them from the string borders since they always represent the math environment

Remove inner $ symbols from latex representations

1f35dcc

* remove them from the string borders since they always represent the math environment * fix tests to check the correct behavior

Add test for a full model that used to be rendered wrong

a4723df

mattiadg marked this pull request as ready for review October 21, 2022 07:46

Merge branch 'main' into fix_latex_print

8088049

# Conflicts: # pymc/tests/test_printing.py

Merge remote-tracking branch 'origin/main' into fix_latex_print

96836cc

# Conflicts: # pymc/tests/test_printing.py

ricardoV94 requested review from larryshamalama and Spaak October 25, 2022 08:26

Spaak requested changes Oct 26, 2022

View reviewed changes

pymc/printing.py Show resolved Hide resolved

pymc/printing.py Outdated Show resolved Hide resolved

mattiadg added 2 commits October 26, 2022 23:36

Fix failing test because of wrong expected value

f072d41

Use "\\operatorname{" instead of "operatorname" to determine if the l…

c306296

…atex command is used

mattiadg mentioned this pull request Oct 26, 2022

Improve Latex representation of symbolic distributions #6202

Closed

mattiadg added 2 commits October 27, 2022 00:49

Merge remote-tracking branch 'origin/main' into fix_latex_print

7867066

Add tests with the code mentioned in the issue

8fe24c4

Spaak approved these changes Oct 27, 2022

View reviewed changes

ricardoV94 reviewed Oct 27, 2022

View reviewed changes

pymc/tests/test_printing.py Outdated Show resolved Hide resolved

larryshamalama approved these changes Oct 27, 2022

View reviewed changes

michaelosthege added this to the v4.3.0 milestone Oct 30, 2022

mattiadg added 2 commits October 30, 2022 21:53

Merge branch 'main' into fix_latex_print

cc3c21c

# Conflicts: # pymc/tests/test_printing.py

Update tests to new distributions' notation

1d5940d

michaelosthege approved these changes Oct 30, 2022

View reviewed changes

michaelosthege merged commit ea721e4 into pymc-devs:main Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix latex repr of symbolic distributions #6231

Fix latex repr of symbolic distributions #6231

mattiadg commented Oct 19, 2022 •

edited

Loading

codecov bot commented Oct 20, 2022 •

edited

Loading

mattiadg commented Oct 24, 2022

mattiadg commented Oct 25, 2022

ricardoV94 commented Oct 25, 2022

mattiadg commented Oct 25, 2022 via email

Spaak left a comment

Spaak left a comment

Spaak commented Oct 27, 2022

mattiadg commented Oct 27, 2022

larryshamalama left a comment

larryshamalama Oct 27, 2022

mattiadg Oct 27, 2022

michaelosthege commented Oct 30, 2022

mattiadg commented Oct 30, 2022 via email

mattiadg commented Oct 30, 2022

michaelosthege commented Oct 30, 2022

		return r"f(" + ", ".join([n.strip("$") for n in names]) + ")"


		def _latex_text_format(text: str) -> str:

Fix latex repr of symbolic distributions #6231

Fix latex repr of symbolic distributions #6231

Conversation

mattiadg commented Oct 19, 2022 • edited Loading

Major / Breaking Changes

Bugfixes / New features

Docs / Maintenance

codecov bot commented Oct 20, 2022 • edited Loading

Codecov Report

mattiadg commented Oct 24, 2022

mattiadg commented Oct 25, 2022

ricardoV94 commented Oct 25, 2022

mattiadg commented Oct 25, 2022 via email

Spaak left a comment

Choose a reason for hiding this comment

Spaak left a comment

Choose a reason for hiding this comment

Spaak commented Oct 27, 2022

mattiadg commented Oct 27, 2022

larryshamalama left a comment

Choose a reason for hiding this comment

larryshamalama Oct 27, 2022

Choose a reason for hiding this comment

mattiadg Oct 27, 2022

Choose a reason for hiding this comment

michaelosthege commented Oct 30, 2022

mattiadg commented Oct 30, 2022 via email

mattiadg commented Oct 30, 2022

michaelosthege commented Oct 30, 2022

mattiadg commented Oct 19, 2022 •

edited

Loading

codecov bot commented Oct 20, 2022 •

edited

Loading