patchGNU Octave - Patches: patch #7969, Organized textread.m for better...

 
 

patch #7969: Organized textread.m for better error notification

Submitter:  Júlio Hoffimann Mendes <juliohm>
Submitted:  Sat 09 Mar 2013 09:25:47 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Wont Do Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 24 Feb 2016 09:27:57 PM UTC, comment #10: 

This patch is becoming moot as a binary textscan is insight - at last ==> status set to "Won't do"

Added a dependency on patch #8783

Philip Nienhuis <philipnienhuis>
Group Member
Mon 18 Mar 2013 07:58:01 PM UTC, comment #9: 

"it'll certainly be for the better" - that's not quite guaranteed, evidenced by several helpful suggestions from you.

It just happens that I wrote a lot of the current textread/textscan/stread code and used it intensively at work, so I got a bit blind to otherwise obvious missing internal documentation and sub-optimal program logic. OTOH I do know a few potential yet undocumented[1] pitfalls, as you have read.

I'd appreciate you scrutinizing when I'm finished (not this week)

What you could do -if you're interested- is to extend the textread.m test suite; at present it comprises only a meager 4 or 5 real tests and it lets a lot of bugs slip through.
Tests could be ripped from textscan.m, which basically functions similarly to textread.

To be cont'd...

[1] rather: hard-to-document-concisely

Philip Nienhuis <philipnienhuis>
Group Member
Sun 17 Mar 2013 08:33:16 PM UTC, comment #8: 

All good points. I probably misunderstood how isindex() works, and you're totally right about clearer warning/error messages.

Feel free to change anything in the patch, it'll certainly be for the better. ;)

Júlio Hoffimann Mendes <juliohm>
Sat 16 Mar 2013 11:52:23 PM UTC, comment #7: 

As it stands, your patch line 60:
 "if (headerlines && isindex (headerlines+1, numel (varargin)))"
won't work reliably:

1. It would introduce a bug as the number of user-specified headerlines bears no relation to the number of args to textread.m (=numel(varargin)+2). It may be better to omit this 2nd arg to isindex() completely. Or did you misunderstand how isindex() works?
Similar issue further below in your patch.

2. isindex (headerlines_value) would rule out a zero value which would still be acceptable (remember these headerlines values could have been computed in other scripts that invoke textread.m). The only thing is that a zero value shouldn't be fed to fskipl(), but that is a textread coder's issue, not a textread user issue.

As to informative-, warning- and error messages:
- Your patch line 47 doesn't explain to user why there are no lines to read.
- Similar reasoning for your patch line 70 (about headerlines value).
- In your patch line 73: ' headerlines' must be a /nonnegative/ number (inform the user of acceptable headerlines values)

We've already discussed EOL issues.

BTW in the textscan.m stanza of your patch there's still a call to textread instead of textscan itself. I'll try to fix that tonight.

My plan:
Using your patch as a guide I'll clean up textread.m as far as reasonable, put the result on the tracker so you can have a look at it, and if you agree on it, I'll push it attributing to you. As textread.m works OK at the moment I can't assign it a very high priority.
Of course you can also come up with a revised patch. But be wary, it seems that textread's test suite doesn't catch all the corner cases, some more tests may be needed.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 12 Mar 2013 08:42:50 PM UTC, comment #6: 

Júlio, I may need a bit more time, I'm also busy with bug #38317

Philip Nienhuis <philipnienhuis>
Group Member
Sun 10 Mar 2013 08:48:58 PM UTC, comment #5: 

Ah... free time. If only.....

I think next Tuesday evening I can dive into your patch.

I was thinking of ripping apart JWE's skeleton and combining it into a stand-alone piece of code. He has done some argument parsing but I figured it may be better to start at processing the text file, leaving input checks & preprocessing to the m-file. Those can be absorbed in C++ later on if needed (the CPU-time consuming part is processing the file).

Philip Nienhuis <philipnienhuis>
Group Member
Sun 10 Mar 2013 08:03:42 PM UTC, comment #4: 

Hi Philip, no problem.

I do have experience with the core, i'm preparing another patch for it right now. ;)

As always, the main problem is free time.

Júlio Hoffimann Mendes <juliohm>
Sun 10 Mar 2013 06:08:32 PM UTC, comment #3: 

Sorry Júlio didn't want to put you off.

textread and strread are on the verge of being deprecated in Matlab - another reason there's not much interest in fixing them.

I'll have a look at your patch later on.

I had another look at JWE's skeleton; to me it looks a bit daunting. It has been written as an extension on existing core routines rather than as a stand-alone function.
Do you have experience in C++ core Octave code?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 10 Mar 2013 01:52:40 AM UTC, comment #2: 

Hi Philip,

Thanks for the comments, all very interesting.

Could you apply the patch and do the changes you have in mind about EOL handling and warnings?

I remember this thread in which John had discussed about performance issues with strread(), the code in there was really hard to maintain. Don't know how deep i can go into this, but i'll be trying slowly.

Thanks.

Júlio Hoffimann Mendes <juliohm>
Sat 09 Mar 2013 11:52:38 PM UTC, comment #1: 

Thanks Júlio for cleaning up some cruft.

Some remarks:

- I don't think it is wise, nor polite, to silently ignore an "improper" ("crazy") user-specified setting for endofline; there should always be a suitable warning if any user setting is deemed inappropriate.
But more to the point, why limit user's choices as to allowable EOLs in the first place?
E.g., Matlab has no strict requirements for what an EOL should look like; recent ML textread docs only say: 'a single character <not specified!> or "\r\n"'.
If you interpret that as the "single character" to only be "\r" or "\n" I'd agree that seems obvious - but it's not strictly according to the docs.
FYI, I've used textread to read old (if not fossil) "direct access" Fortran (IIRC) files where line endings comprised special bytes (often null bytes) or byte sequences. Admittedly a bit arcane but it does show there's a use for unintuitive EOLs. (It may look weird but IMO there's nothing wrong with trying to invoke textread or textscan to recover old data from almost binary data files that were produced with long gone applications. Think of database files where "traditional" EOLs can't be used to delimit records if some fields can contain multi-line text)

- As to performance: I don't think there's much speed to gain in textread.m (and for that matter, textscan.m). They're just front-ends.
If you want to really increase performance, please set yourself to producing a compiled (binary) textscan.cc
AFAIK it was (is) intended to have a compiled (x)textscan.cc as backend for strread textread and textscan itself.
JWE posted a skeleton some time ago (around August 4, 2011 in a message with subject "strread.m").

The current workhorse strread.m is a dinosaur; I think it was set up the way it was because that was the only way it could be efficiently vectorized; yet that setup imposes several limitations. Over time, adding more options made it a complicated animal until Rik and I agreed that spending more effort at it was pointless; we're all waiting for a compiled backend.
Some initial profiling last year showed that -as expected- most of the CPU time is lost in the various nested if-elseif-else-endif sequences and cellfun() operations on data columns that are unavoidable because text files have virtually unlimited variability.
If you're not put off by this (hopefully not) I'm at your disposal because lots if not most of the current code in strread.m was added by me.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 09 Mar 2013 09:25:47 PM UTC, original submission:  

The code was cleaned up and passed all previously existing tests.

I'm planning to improve its performance, this patch is just to make things clear before the hard work.

Júlio Hoffimann Mendes <juliohm>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #27587:  textread.patch added by juliohm (7KiB - text/x-patch)

 

Digest:
   patch dependencies.

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by juliohm (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-06 lachlan Open/ClosedOpen Closed
    2016-02-24 philipnienhuis StatusNone Wont Do
        Dependencies- Depends on patch #8783
    2013-03-09 juliohm Attached File- Added textread.patch, #27587

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code