Skip to content

Fix aastex lastline #5236

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.

Sign up for GitHub

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

Jump to bottom
Closed
wmwv wants to merge 3 commits into from
Closed

Fix aastex lastline #5236

wmwv wants to merge 3 commits into from

Conversation

@wmwv
Copy link
Contributor

@wmwv wmwv commented Aug 11, 2016

Allow for last line to not have an ended \\ by restructuring the LatexHeader line-by-line generator and write a special-case last_line option for processing the lines of a table.

The tests for this are part of the round-tripping testing of #5235.

wmwv added 3 commits August 11, 2016 16:18
@wmwv
Minor typo fixes.
4225349
@wmwv
Override __call__ method to handle last line separately.
024f0bb 
LatexSplitter inherits from core.BaseSplitter
LatexSplitter.__call__ overrides the default
function to handle the last line with a special case that allows
for a lat line without \\.
@wmwv
Add last_line option to LatexSplitter.process_line.
06c63c0 
This requires adding the same to any class that inherits from LatexSplitter.
In this case this also required adding last_line to AASTexHeaderSplitter.process_line.
even though it doesn't do anything in the case of reading a header.
@wmwv
Copy link
Contributor Author

wmwv commented Aug 11, 2016

@taldcroft Could you take a look at this pull request to allow the reading of LaTeX table files with no \\ on the end?

@pllim pllim added the io.ascii label Aug 11, 2016
@wmwv wmwv mentioned this pull request Aug 11, 2016
Closed
@astrofrog
Copy link
Member

astrofrog commented Aug 12, 2016

@hamogu - you may also be able to review this?

@hamogu
Copy link
Member

hamogu commented Aug 12, 2016

This should have an entry in CHANGES.rst.
I would merge this content into #5235. We are always reluctant to merge something that's not tested yet and we can't just merge #5235 because that would make the tests fail until this is merged. Can we just have both code changes be part of the same pull request since one of them solves the problem and the other one tests it?

@wmwv
Copy link
Contributor Author

wmwv commented Aug 12, 2016

#5235 tests still fail because it can't read the \begin{tabletype}[tablealign].

To help keep things cleaner, I'll add a basic separate reading test in test_read.py for this present PR and will do the same in #5237 .

@taldcroft
Copy link
Member

taldcroft commented Aug 16, 2016

@wmwv - just back from vacation and trying to catch up.

MSeifert04
MSeifert04 reviewed Sep 9, 2016
View reviewed changes
astropy/io/ascii/latex.py
def find_latex_line(lines, latex):
'''
Find the first line which matches a patters
Find the first line which matches a pattern
Copy link
Contributor

@MSeifert04 MSeifert04 Sep 9, 2016

Choose a reason for hiding this comment

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

This is missing a . at the end of the sentence.

@MSeifert04
Copy link
Contributor

MSeifert04 commented Sep 9, 2016

I included some comments but this seems really complex for such a small change. For example you override __call__ but it seems you only altered one line compared to the BaseSplitter.__call__. Wouldn't it be possible to use super()?

@MSeifert04
Copy link
Contributor

MSeifert04 commented Sep 9, 2016

As alternative: Why not just check the "last line" if it "ends" with a \\ and if not append one?

@hamogu
Copy link
Member

hamogu commented Sep 10, 2016

Thanks @MSeifert04 for a very through review. Some of the changes recommended as easy and should definitely be done (adding . at the end of sentence etc.) while we are looking at this in detail right now.
In some cases, I think the suggestions are more stylistic. Personally I don't care if this uses enumerate or generators or itertools.isslice. I bet the performance impact in a typical analysis session is low even in the worst case (yes, I'm sure you can timit by reading the same table 1000 so, times, but in real live nobody does that).
So, @wmwv, it be great if you look at all suggestions, but if you decide to not implement some of them and leave as is, that's totally fine with me, too, and I would still suggest to merge this soon (but the final say over this matter is with @taldcroft ).

@taldcroft
Copy link
Member

taldcroft commented Sep 10, 2016

I agree with @MSeifert04 that this change appears at the outset to be more complicated than necessary. Following on his suggestion, would this work?

def __call__(self, lines):
    last_line = lines[-1].split('%')[0].strip()
    if not last_line.endswith(r'\\'):
        lines[-1] = last_line + r'\\'

    return super(LatexSplitter, self).__call__(lines)

pllim
pllim reviewed Sep 10, 2016
View reviewed changes
astropy/io/ascii/latex.py

class LatexSplitter(core.BaseSplitter):
'''Split LaTeX table date. Default delimiter is `&`.
'''Split LaTeX table data. Default delimiter is `&`.
Copy link
Member

@pllim pllim Sep 10, 2016

Choose a reason for hiding this comment

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

& should be using double ticks, not single.

@taldcroft taldcroft mentioned this pull request Sep 12, 2016
Closed
@taldcroft taldcroft mentioned this pull request Sep 19, 2016
Merged
@taldcroft taldcroft mentioned this pull request Oct 22, 2016
Merged
@hamogu hamogu mentioned this pull request Dec 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

io.ascii Ready-for-final-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wmwv @astrofrog @hamogu @taldcroft @MSeifert04 @pllim