search

admb-project / admb

AD Model Builder
http://admb-project.org
Other
64 stars 19 forks source link

make ADMB data file parser robust to different line endings #156

Closed iantaylor-NOAA closed 3 years ago

iantaylor-NOAA commented 3 years ago

A recent discussion about issues caused by line endings in model input files led me to this 2012 thread: http://lists.admb-project.org/pipermail/users/2012-January/001659.html where @arni-magnusson notes that errors can arise when

  1. Model is run in Linux
  2. Data file has Dos line endings
  3. Data file has empty line between two comments

and suggests that

I hope the long-term solution will be to make ADMB data file parser more robust.

Based on what I'm hearing from others, this issue remains in recent ADMB versions.

@johnoel, feel free to close this if a fix would be too hard (or if it's already been fixed)

johnoel commented 3 years ago

Hi @iantaylor-NOAA,

The input stream was simplified and improved to work with windows and unix line endings. Also, the issue reading a data file with the empty line between comments has been fixed. Small test cases showed that the changes resolved the issues, but it would be nice to get additional input files to test with.

iantaylor-NOAA commented 3 years ago

Thanks @johnoel and good to hear that the input stream was improved. I'll see if we can track down a reproducible case which occurs with a recent version of ADMB. Maybe this is just a red herring.

@jimianelli and @lee-qi, did I hear correctly that you were having this issue with Stock Synthesis input files run on a Mac? Recent SS executables have been compiled in ADMB-12.2, but you should be able to confirm this using a command like ./ss -version.

lee-qi commented 3 years ago

Hi! This came up as an issue in our SS workshop when someone inadvertently saved a data file with Mac OS 9 line endings. I think what happened was they copied data from an Excel spreadsheet and pasted onto Sublime Text before saving it as a data.ss file.

The error showed up as Error -- Maximum line_adstring length exceeded in istream& operator>>(istream&, line_adstring&).

It's easily fixable in Sublime, but it was confusing to debug. I've tested that same file using Unix and Windows line endings, and it works fine. Happy to share the file if it'd be useful.

I'm on a Mac (OS Version 11.0.1), using an SS executable compiled with ADMB-12.2, built Jul 31 2020. I've personally never encountered something like this before, and have been using a similar workflow on a Mac for a while now.

johnoel commented 3 years ago

Thanks @lee-qi for detailing the issue. I will add a check for older MacOS 9 line endings. Let me know if you can help with testing.

lee-qi commented 3 years ago

Happy to help with testing. Just let me know!

On Fri, Dec 11, 2020 at 3:01 PM johnoel notifications@github.com wrote:

Thanks @lee-qi https://github.com/lee-qi for detailing the issue. I will add a check for older MacOS 9 line endings. Let me know if you can help with testing.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/admb-project/admb/issues/156#issuecomment-743470311, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACRO5LTKMYVSWBVWK7BJLILSUKQB3ANCNFSM4USWDJBA .

johnoel commented 3 years ago

Hi @arni-magnusson, @jimianelli, @lee-qi and @iantaylor-NOAA, The issue156b branch has the changes to correct data inputs for three types of line endings including the ones for MacOS 9. Please test to check if the branch works for you.

lee-qi commented 3 years ago

Hi, I couldn't test the exact issue that had brought this up (because I don't have the SS.tpl to compile). That said, I tested this with a different set of files using both the old compiler and the new one. Mac OS 9 line endings in the data files didn't work for the executable compiled with my original compiler (no actual error; just kept going without any output on the console), but the one compiled with the new branch worked as expected. Also confirmed that it worked with Windows and Unix line endings. Cheers!

k-doering-NOAA commented 3 years ago

@lee-qi , just in case you do want to test by compiling stock synthesis, this is the latest release tpl. You can find a few older versions in the same folder. Thanks for the tests you have done so far!

lee-qi commented 3 years ago

Thanks, Kathryn!

Huh. Looks like it didn't work for SS. Got the same error (Error -- Maximum line_adstring length exceeded in istream& operator>>(istream&, line_adstring&)) whether SS was compiled with the new ADMB version vs the old. They work fine when the files were saved with Unix / Windows line endings.

Happy to email the files to folks, if that would help. They're basically just starter.ss files saved with Unix and Mac OS 9 line endings.

k-doering-NOAA commented 3 years ago

@lee-qi, sounds like a problem on the SS side then, thanks for checking this. Can you email the files to nmfs.stock.synthesis@noaa.gov?

Rick-Methot-NOAA commented 3 years ago

When I looked at the unix vs Mac files in Hex mode, I see that the line endings are different. Could this be the source of the problem?

Rick-Methot-NOAA commented 3 years ago

Lee mentioned that the offending file came from Excel originally.

I also have seen issues when copying from Excel to text and then reading the text with ADMB-SS. I think it is Excel creating the offending line endings.  It is probably a cell ending code and not actually a line ending code. Perhaps Excel on the Mac is the source of the problem.

johnoel commented 3 years ago

Hi @lee-qi and @k-doering-NOAA, Can you send me that starter.ss file as well?

johnoel commented 3 years ago

Still need to redo cifstream::signature for \r line endings...

johnoel commented 3 years ago

Hi @lee-qi and @k-doering-NOAA, can you please test latest changes from the issue156b branch to support the \r line endings?

k-doering-NOAA commented 3 years ago

I just tested compiling SS with the issue156b branch of admb and am still getting the same error that @lee-qi has been seeing:

Error -- Maximum line_adstring length exceeded in istream& operator>>(istream&, line_adstring&)

Could it be something on the Stock Synthesis side rather than ADMB? I have no idea, but perhaps you do, @johnoel ?

johnoel commented 3 years ago

Opps, I meant the issue156 branch not issue156b. Just tested and it works with the files you sent.

k-doering-NOAA commented 3 years ago

Using issue156 branch worked for me, also!

johnoel commented 3 years ago

Good, it was mainly an issue in admb which needed to support the macos9 line endings. Thank you for helping me with the testing.

lee-qi commented 3 years ago

Bit belated, but quick update to say that it worked for my machine too. Thanks, @johnoel!