Add _repr_latex_ method to table.Table and test for it, fix for #3972 - #11508

Thanks for the PR! Recently, the change log style has been changed, so CHANGES.rst should not be touched anymore. You should instead follow the instructions in the Changes README.

Also, the code style checks show that there are some errors. You can check there which lines have issues. Essentially, we follow PEP8 conventions to keep the code well organised; this includes removing any trailing white-space, following maximum line length, etc. There are guidelines here that you can follow (you can even check code style before you push code)! Hopefully this solves some of the errors so tests can fully run.

Hey, thanks for your comments.
I made some changes, however a few tests are still failing.
Are these allowed to fail or is there anything I can do about it?

Thanks!

Yes, the failed tests are unrelated. The milestone error is because we still need to set a milestone to this PR. So you don't need to worry about it. I will do a detailed review soon.

Thank you for your review and the detailed suggestions.
I have made changes to the code.

Here are the suggested edits.
Let me know if it would be required to rebase the commits.

OK, almost done. I noted that there is no test for the case when we have a meta['latex_dict']. That got me thinking and the more I think about it, the more I wonder why this function accepts arguments at all. What you did is great, but I just read a big article on why humans tend to add things and make things more complicated to fix a problem instead of taking something away. The same is true for code - the best (and hardest!) PRs are often those that solve a problem by taking something way.

We already have a function to write a LaTeX table with options: Table.write(..., format='ascii.latex', latex_dict=...). If anyone wants to pass in options and call a writing method by hand, they should use that one and not one that starts with an underscore (those are considered private methods). That means, that this method should not accept the latex_dict. This method is for being called automatically, with no options.

So, I'm asking you to make the following change:
If there is a meta['latex_dict'], then use that, if not, use the default.

That simplifies the code, the docs, and the tests (only two cases to test, wich and without meta). In the end, you'll probably have only half the lines that you have now. Don't worry for GSOC, we've seen that you can do the complicated stuff!

I'm sorry that this is such a long and winding journey where the things we ask for change with time. I promise this is the last thing. You might wonder why I did not say that in the very beginning. One of the problems with code review is that sometimes I only realize what astropy needs until someone has written a draft to look at and try out.

Hey, thanks for your comment.
I see the point that you make. In fact, it had also crossed my mind that the extra advantage that using this function has on top of Table().write wasn't much. From a user perspective, it would make more sense to use Table().write. I agree that the _repr_latex_ method should be designed for internal usage and automated execution.

I have now made changes to the function following your suggestions. _repr_latex_ doesn't take any parameters now and the latexdict takes the default value if no Table.meta['latexdict'] is present. However, the test for a case with Table.meta['latexdict'] defined before the call to _repr_latex_ function was being done using lTable3 towards the end of the test_repr_latex() in test_pprint.py. This has been retained.

Great!
I think the changes are good to go now.

With apologies to all involved, I'd like to recommend reverting this PR for the 4.3 release. Please note @eteq.

This has caused some unexpected problems related to Jupyter notebooks for reasons that one never would have guessed. In short, the new rich display method _repr_latex_ for Table gets evaluated every time a Table is displayed in Jupyter notebook (see https://stackoverflow.com/questions/45560556/what-is-the-precedence-order-of-jupyter-rich-representations). This has problems:

• We already saw Array columns error in Jupyter notebook and lab #11695.
• I noticed weird problems in the VS code Jupyter extension which I now understand are the result of this PR.
• The way Jupyter works is that it evaluates all of the rich display methods and then picks one of them. This PR uses the latex writer without considering the table length, so if the user has a Table with 100 million rows, it will take a long time and lots of memory to generate an internal LaTeX representation, which then gets ignored.

I think the original use case from @Cadair in #3972 was pretty narrow and that issue has not gotten any other engagement in 6 years.

OK, I agree. Let's revert this. Jupyter is used much more than #3972. I'm glad we got this in sufficiently before the 4.3 freeze, so that there was some real world use beyond our tests to uncover this problem before it made it into a released version.

I see several ways of fixing this particular problem (for example the LaTeX table could be limited to the top x rows similar to what is done in the html display) but I agree @Cadair is initial use case is too narrow to warrant that extra complexity though. I do wonder about the notebook and "export to pdf via LaTeX" in nbconvert now. Presumably, for that purpose we do want a latex representation of tables (I don't know which one it pick now, need to check), so let's not throw out this code altogether. No hurry for 4.3, that can be released with this reverted, but we just might have to look at this again after that.

I think this went into the v4.3.x branch, so I think we need @eteq to do the revert.