Ticket #7732 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

SaveAscii and LoadAscii multiple spectra format needs to be changed

Reported by: Keith Brown Owned by: Keith Brown
Priority: major Milestone: Release 3.1
Component: Framework Keywords:
Cc: Blocked By: #8374
Blocking: #3493, #7645 Tester: Karl Palmen

Description (last modified by Keith Brown) (diff)

SaveAscii outputs a different format if the data is one spectrum rather than multiple. The single spectrum format is good, but the multiple format is useless, not used and defined badly years ago.

Create a new version of Save and Load Ascii, probably inheriting from the first where beneficial, that output multiple spectra by using the single spectra format appended multiple times in the file, with a header to specify the spectra and workspace index.

Change History

comment:1 Changed 7 years ago by Keith Brown

  • Description modified (diff)

comment:2 Changed 7 years ago by Keith Brown

  • Blocking 4464 removed

(In #4464) Advised by nick that this is a failure case as if SaveASCII has been altered, LoadASCII needs equivalent changes made to it so that the saved file can be loaded.

#7732 isn't blocking any more as the error is totally separate.

comment:2 Changed 7 years ago by Nick Draper

  • Owner set to Keith Brown
  • Description modified (diff)
  • Summary changed from SaveAscii saving files incorrectly to SaveAscii and LoadAscii multiple spectra format needs to be changed

comment:3 Changed 7 years ago by Karl Palmen

  • Blocking 3493 added

(In #3493) This ticket only applies to mult-spectra ASCII files. The current format for multi-spectra files is not considered to be good and will be revised in ticket #7732. Then it may be easier to support ragged workspaces. It does not makes sense to support ragged workspaces while the multi-spectra format is not good, hence this ticket is blocked by #7732.

comment:5 Changed 7 years ago by Keith Brown

  • Status changed from new to inprogress

Added new files to Cmake

Created new copies of the save and load Ascii algorithms so i have a base for writing version 2.

They have been added to cmake

Refs #7732

Changeset: 3316a8c66d254e8def68535cbec1036941836300

comment:6 Changed 7 years ago by Keith Brown

Now saves in the desired format

The data is now saved in the follwing format with each spectra grouped together:

{spectrum number}

[optional]{comment marker}X,Y,E[optional],DX

X(0,0){separator}Y(0,0){separator}Z(0,0)[optional]{separator}DX(0,0)

X(0,1){separator}Y(0,1){separator}Z(0,1)[optional]{separator}DX(0,1)

...

{spectrum number}

...

Refs #7732

Changeset: d17aadd57501eb250075837bb59f43f063406729

Last edited 7 years ago by Keith Brown (previous) (diff)

comment:7 Changed 7 years ago by Keith Brown

Created Test classes and added to CMake

Copied the original tests so i have a base for my own

Refs #7732

Changeset: f6421181f55383605f19c9ae4882e50a5921c234

comment:8 Changed 7 years ago by Keith Brown

Made Basic changes to headers and loadascii2's cpp

Changed thigns like the version, author etc. no actual functional changes

Refs #7732

Changeset: 8366bd044916db6fd3259eeb2e699804fde823b4

comment:9 Changed 7 years ago by Keith Brown

Lowered version 1 confidence

I've lowered teh defualt confidence of LoadAscii version 1 as it was preventing LoadAscii version 2 from being chosen by the Load selector

Refs #7732

Changeset: a2f513d0a35e6df32da11580558cba6138618ef6

comment:10 Changed 7 years ago by Keith Brown

Altered LoadAscii Cumstom dialog

The LoadAscii Dialog now changes depending on the version being invoked.

Version 2 has the new comment indicator feild

Refs #7732

Changeset: d6ef0ab66a6fee7c2af0baa6d41ba3bb7e8b7ab7

comment:11 Changed 7 years ago by Keith Brown

Tided code and formatted

Removed some commented out code and some trailing spaces

Performed a "format Document" command form Visual studio to add indentation.

Refs #7732

Changeset: 724b0c29efc390ebb3a7018a1c1a1ae8fb189741

comment:12 Changed 7 years ago by Keith Brown

Added new format, removed debug code

Added the new Scientific Notation option to the dialog

Removed the std::cerr calls I added when debugging

Changed the placeing of the header code so that the header is no longer per specta, but per file

Refs #7732

Changeset: aca508ffe61e7930a8c2ba95890718ace30271f6

comment:13 Changed 7 years ago by Keith Brown

Written the unit test for version 2

Createa s unit test for the new format based on the old test.

Compared to the original the unit test is now split into two.

Refs #7732

Changeset: 1ab37467b6737f36aca07a1d8ee3b6dcaadbbba2

comment:14 Changed 7 years ago by Keith Brown

New version of LaodAscii written

The new version of LoadAscii was going to be based on the original, and it has been, but not as much as was initially intended, and it ended up a full rewrite.

The Loader will load the form of files saved by SaveAscii version 2, but not version 1. The format has changed that much.

It is slightly flexible in that it will load files that SaveAscii wouldn't write, but this only extends as far as it'll accept blank lines as equivalent to spectra IDs, will allow 2 columns minimum (implying that E and Dx are 0), and will ignore lines starting with a comment character. However, it is still strict enough that it will expect any standards set at the start of the file to be continued throughout. This means number of bins per spectra, number of columns per bin, inclusion or omittance of spectra IDs, comment characters and delimiters.

Refs #7732

Changeset: 8b2f96d93735a72b2dec943390730bf579badc29

Last edited 7 years ago by Keith Brown (previous) (diff)

comment:15 Changed 7 years ago by Keith Brown

Clarified some errors and comments

Errors now include the line number in the file if it's avialable to them.

Some comments have been deleted or improved

Refs #7732

Changeset: 237190c02623d88ccf0f791d50c395bbfd28e76b

comment:16 Changed 7 years ago by Keith Brown

Refactored Save Algorithm

SaveAscii now validates the comment indicator and user defined separator.

The code has been refactored to split off the spectra writing code into a mthod with two overloads: one taking an int and one taking an iterator.

Some varables have become private member variables to help with the new methods

Refs #7732

Changeset: abf05e0a29d09dbe65b45aae96fd6e85b9a0385f

comment:17 Changed 7 years ago by Keith Brown

Load Algorithm now compatible with custom separators

The Save Algorithm had always been able to define a custom spearator, but ht eLoad algorithm wouldn't recognise them.

Custom separator code now added to both algorithm and custom dialog.

Refs #7732

Changeset: 2c48d3aa909f28c4554cac0fca0d2bb4a624005f

comment:18 Changed 7 years ago by Keith Brown

Removed a redundant method, and corrected some things.

isAscii() was originally included as I copied LoadAscii v1 originally, and as it's not actually used internally i've removed it form v2. v1's isAscii is only used once (and by another class), and LoadAscii (the v1 class) is specifically used then, so it's safe to remove it from v2. In theory, that call to v1 should probably be using a file descriptor instead.

The spacers 2d array now has it's bounds corrected for both the new item, and reducing the second dimension as much wasn't used.

The Filename property has had an error in it's description corrected.

Refs #7732

Changeset: 3bfa2fdbf0fab7b27b75d00c35b3a72b03969c82

comment:19 Changed 7 years ago by Keith Brown

Changed the type of exception thrown for spearators and comments

Save and Laod now both throw std::invalid_argument rather than std::runtime_error if validation on separator and comment characters fail.

Refs #7732

Changeset: d86a60862e41ae3ed8343a7b8d70d24f90d770f9

comment:20 Changed 7 years ago by Keith Brown

Added value checking to SaveAscii2Test

SaveAscii2Test now checks some values.

Refs #7732

Changeset: a19eb584d88ed5f472245140487d9f82e6664b85

comment:21 Changed 7 years ago by Keith Brown

Started LoadAscii2Test

Rewritten the checkData and runtest methods as well as amalgamated the file writing methods into one method.

Written some test cases, no fail cases yet.

Refs #7732

Changeset: 0cd0d62cc4d15c5d6d622f9c16a99f0a66c69222

comment:22 Changed 7 years ago by Keith Brown

Improved validation and other features of Save

Save now closes the ostream. It didn't before and it's a wonder why it never crashed.

The comment and separator validation has been improved to disallow - + and e as they're part of the scientific notation. the separator = comment check is now bundled into the comment's regex check.

Spectra min/max have now got a positive check.

Headers now use the specified spearator character.

Refs #7732

Changeset: 61b68751e1e7e11a4e6af93d5be1e60e6d0e025f

comment:23 Changed 7 years ago by Keith Brown

Various fixes to Load

Improved some long lines.

Fixed an issue where all the separator options weren't being shown.

CustomSeparator now has the same property settings as it's SaveAscii2 counterpart.

The comment and separator validation has been improved to disallow - + and e as they're part of the scientific notation. The separator = comment check is now bundled into the comment's regex check.

Refs #7732

Changeset: e5ca377c4e4768731c22bc6a84b26fbdbb0a17b8

comment:24 Changed 7 years ago by Keith Brown

Lots of imporvemetns ot SaveAscii2Test

Two repeated blocks of code are now factored into their own methods: Writing the sample WS, and initialising SaveAscii2.

Made the filename and WS name variables global.

Added many more tests to check fail cases.

Refs #7732

Changeset: 18a04a573372a102988ce7de5218aa8323385a66

comment:25 Changed 7 years ago by Keith Brown

Imporved LoadAscii2 Test

Written failure cases

Refactored the data check method.

Added another paramerter to runTest.

Cleaned a few lines up.

Refs #7732

Changeset: ed14a7e63e5c59f4ef79cab1594438bdc0772dcd

comment:26 Changed 7 years ago by Keith Brown

Added a new test to SaveAscii2Test

The new test tests the WriteXError flag

Refs #7732

Changeset: 3b4212507ac3d71242142a3b9f06b11e4bb70058

comment:27 Changed 7 years ago by Keith Brown

Merge branch 'feature/7732' into develop

Conflicts:

Code/Mantid/Framework/DataHandling/CMakeLists.txt

refs #7732

Changeset: 7f2f0d85741a7257edfd983a18403360cdf4d06e

comment:28 Changed 7 years ago by Keith Brown

Fixing various gcc compiler errors

Changed a nuber of variables from int to size_t.

Changed instanced of std::to_string to boost::lexical_cast<std::string>

Cahnged a call to getProperty so taht it assigned to a new varaible.

Refs #7732

Changeset: 69495fba6896943fc34b3ce5186e31d093e025af

comment:29 Changed 7 years ago by Keith Brown

  • Status changed from inprogress to verify
  • Resolution set to fixed

The format has changed so that each group of rows is a single spectra (where each row is one bin for each of the axis). The rows are sequential in that they're shown going from left to right when loaded into mantidplot

The format must be:

  • A single integer or blank line to denote a new spectra
  • For each bin, between two and four columns of delimted data in the following order: 1st column=X, 2nd column=Y, 3rd column=E, 4th column=DX (X error)
  • Comments can be included by prefixing the line with a non-numerical character which must be consistant throughout the file and specified when you load the file. Defaults to "#"
  • The number of bins is defined by the number of rows and must be identical for each spectra

The following is an example valid file of 4 spectra of 2 bins each with no X error
# X , Y , E
1
2.00000000,2.00000000,1.00000000
4.00000000,1.00000000,1.00000000
2
2.00000000,5.00000000,2.00000000
4.00000000,4.00000000,2.00000000
3
2.00000000,3.00000000,1.00000000
4.00000000,0.00000000,0.00000000
4
2.00000000,0.00000000,0.00000000
4.00000000,0.00000000,0.00000000

To Tester

Load any workspace, preferably a large one.

Save in every format available to you through SaveAscii v2 with all combinations of options. Then load the data back into mantid using LoadAscii v2, making sure the data is accurate and in the right place.

Also make edits to the saved files to test the flexibility/strictness of LoadAscii v2 as per the allowances mentioned in comment 14

comment:30 Changed 7 years ago by Keith Brown

  • Status changed from verify to reopened
  • Resolution fixed deleted

need to fix gcc compiler warnings again....

Last edited 7 years ago by Keith Brown (previous) (diff)

comment:31 Changed 7 years ago by Keith Brown

  • Status changed from reopened to inprogress

Disabled algorithms to stabilize build servers

Some unit tests were relying on these algorithms, but they were failing because load wasn't accepting the files.

Load should be albe to load single spectra files formt he old format so i'm going to investigate why not now.

the bizarre thing is that this didn't occur on windows when i ran the tests locally.

Refs #7732

Changeset: 2295e1df47230128e1b01366966b260ce45b37fe

comment:32 Changed 7 years ago by Keith Brown

Found out what it is.

All those files have header info that was never used by mantid (loadascii v1 just skipped over it), my new implementation just plain doesn't like any non-data without leading comment characters.

Basically when I re-wrote both of them I wrote save first which didn't wirte any header info, as version 1 didn't include any header info either. Thus when I wrote Load I wrote it as if it were only going to load files form SaveAscii version 2, with a few exceptions. I didn't realise that the "processHeader" function I removed (or more accurately didn't re-implement) was actually important in that it allowed the program to load files no written with mantid's internal Ascii writer as the files the system tests failed on must have all had that header info.

I'll re-implement processheader (or add this functionality to setcolumns()), which will ignore either a set amount of line supplied by the user, or find the first data line and go from there

comment:33 Changed 7 years ago by Keith Brown

Working on getting the old single format recognised

Just started, reenabled the algorithms

Refs #7732

Changeset: f8f2663f5efee0d278ffc7e434416422d51af6b7

comment:34 Changed 7 years ago by Keith Brown

Changed SaveAscii2Test Filenames

The test was conflicting with the original version due to filenames, changed the filename V2 uses.

Refs #7732

Changeset: d95f541bf2044e5d6f6e3b860ae3f052e149e259

comment:35 Changed 7 years ago by Keith Brown

Refs #7732 Readded processheader with a few modifications

Readded processheader with additions to make it accept the spectra IDs in the new format

Changeset: 25c06cd82378cfb48ebbdd5c1ba398ed30630dff

comment:36 Changed 7 years ago by Keith Brown

refs #7732 tided up some comments

Removed some comments form when i was making the edit

Changeset: 0a44305b180126c239435dcd008d0f8764b85327

comment:37 Changed 7 years ago by Keith Brown

Refs #7732 disabled save/LoadAscii2 again

Disabled save/LoadAscii2 again due to a systemtest i failed to fix: LoadLotsOfFiles

Changeset: 50d55897a1ff539c39859812688fea258c56caff

comment:38 Changed 7 years ago by Keith Brown

  • Milestone changed from Release 3.0 to Release 3.1

comment:39 Changed 7 years ago by Keith Brown

Refs #7732 Changed LoadLotsOfFiles System Test

The changes to LoadAscii highlighted a problem with LoadLotsOfFiles where it was also loading .cal files when it shouldn't.

Added a blanket ban on .cal files and changed the regex search to be case insensitive.

Changeset: dcf7a273c0354b103fdfeacb6a2c6e27c5beedc7

comment:40 Changed 7 years ago by Keith Brown

Refs #7732 re enabled the algoithms again

Changeset: ff384a8d56eda303bc16587620cd587003a23190

comment:41 Changed 7 years ago by Keith Brown

Refs #7732 Merge master into my branch

Conflicts:

Code/Mantid/Framework/DataHandling/CMakeLists.txt

Changeset: 238531bf0b0e084a8d098bb9ee51f4c8ffb8f13a

comment:42 Changed 7 years ago by Keith Brown

Refs #7732 fixing compiler warnings on linux

Changeset: 674602fbf5573adb37d5fa2884869ffc61a92342

comment:43 Changed 7 years ago by Keith Brown

Refs #7732 fix compiler warning

Id cahnged an int to a size_t, and there was a "if negative" check that would always return false with the new type

Changeset: 7ea0f5db4e2a55da702423caf05c3f6d16f5b758

comment:44 Changed 7 years ago by Keith Brown

Refs #7732 ran the VS auto formatter on my new files

Changeset: 6763e3b06b381ee8894750d1c8cab630531383d4

comment:45 Changed 7 years ago by Keith Brown

Refs #7732 added .detcal to banned Regex in LoadLotsOfFiles

Another file that shouldn't be loaded was found, added .detcal to the banned list.

Re-added the .cal blanket ban after a merge from master overwrote it.

Changeset: 08eb9e4ece176e6e16d1d7c5aca46d0412f13f8f

comment:46 Changed 7 years ago by Keith Brown

This should be ready for testing again as it's not failing system tests any more and it'll load the single-spectra files with header info that v1 could.

To Tester:

Follow the instructions in the original "To Tester" in #comment:29. Also load any single spectra ASCII files you can find in the system tests, auto tests or any other source you have available to you to make sure that this can still open them.

Multiple spectra files using the old format aren't supported.

comment:47 Changed 7 years ago by Keith Brown

  • Status changed from inprogress to verify
  • Resolution set to fixed

comment:48 Changed 7 years ago by Russell Taylor

  • Status changed from verify to reopened
  • Resolution fixed deleted

The LoadLotsOfFiles system test is still failing on a number of HFIR SANS files (*Iqxy.dat).

P.S. You've also spelt beginning wrong (beggining) in a couple of places, which should be fixed even though it was useful in pinning down the source of the failures!

comment:49 Changed 7 years ago by Karl Palmen

  • Blocking 7645 added

(In #7645) The situation may have changed as a result of work on ticket #7732. This will need checking and work done, if necessary.

comment:50 Changed 7 years ago by Keith Brown

  • Status changed from reopened to inprogress

Refs #7732 adding a setting to HFIRTestsAPIv2 to see if it cleans up the mess.

Added the NoSaveIq() setting to the reductions in HFIRTestsAPIv2 to see it that stops it leaving fiels around. Not totally sure if this will work as i can't see if the test requires those files or not.

Changeset: 24c4990b5acb6e29d7660c0ee89b703c97766053

comment:51 Changed 7 years ago by Keith Brown

  • Blocked By 8374 added

#8374 is blocking this as the system tests need cleaning before this will pass them.

comment:52 Changed 7 years ago by Keith Brown

Revert "Refs #7732 adding a setting to HFIRTestsAPIv2 to see if it cleans up the mess."

This reverts commit 24c4990b5acb6e29d7660c0ee89b703c97766053.

Changeset: 609ec89a346b402f2686ee2cb90158a90ce61739

comment:53 Changed 7 years ago by Keith Brown

Refs #7732 LoadAscii2 now allows - and + at start of line

A slight mistake whenre i was only checking for digits at the start of valid lines. The Algorithm will now allow + and - if supplied to determine positive and negative. lineNo is now a member variable in order to fix line number accuracy after the changes with processHeader made them incorrect.

Changeset: 28abc059f5fd9029a1ff2de191bc2562c8558a9a

comment:54 Changed 7 years ago by Keith Brown

The systemtests should have been fixed in #8374 so this should be tested after that is passed.

comment:55 Changed 7 years ago by Keith Brown

  • Status changed from inprogress to verify
  • Resolution set to fixed

The system tests that were failing when this was merged to develop previously were generating artefacts that LoadLotsOfFiles was trying to load with LoadAscii, even if the files weren't suitable.

To Tester:

Load any workspace, preferably a large one.

Save in every format available to you through SaveAscii v2 with all combinations of options. Then load the data back into mantid using LoadAscii v2, making sure the data is accurate and in the right place.

Also make edits to the saved files to test the flexibility/strictness of LoadAscii v2 as per the allowances mentioned in ​#comment:14

Also load any single spectra ASCII files you can find in the system tests, auto tests or any other source you have available to you to make sure that this can still open them.

Multiple spectra files using the old format aren't supported.

Note that this also included edits to the systemtests repository when I was banning specific types of files. So they will also need merging if this passes.

comment:56 Changed 7 years ago by Martyn Gigg

  • Status changed from verify to reopened
  • Resolution fixed deleted

Could you address this compiler warning and then reclose the ticket.

comment:57 Changed 7 years ago by Keith Brown

  • Status changed from reopened to inprogress

Refs #7732 Fixed Ubuntu compiler warning

Removed a set but unread varable and a redundant else if clause that was causing an ubuntu compiler warning.

Changeset: 65930d795d55e004b3c39711ac7a10d2e724cf3f

comment:58 Changed 7 years ago by Keith Brown

  • Status changed from inprogress to verify
  • Resolution set to fixed
  • Description modified (diff)

comment:59 Changed 7 years ago by Karl Palmen

  • Status changed from verify to verifying
  • Tester set to Karl Palmen

comment:60 Changed 7 years ago by Karl Palmen

  • Status changed from verifying to closed

Merge remote-tracking branch 'origin/feature/7732_Load_Save_Ascii_Multiple_Spectra'

Full changeset: 17e23f036a85fa602e09da721c71aef56d066b87

comment:61 Changed 7 years ago by Karl Palmen

Merge remote-tracking branch 'origin/feature/7732_Load_Save_Ascii_Multiple_Spectra'

Full changeset: bb879833f0c3cf44076920ba7e4293b32a8140d3

comment:62 Changed 7 years ago by Karl Palmen

The second of the two commits just done was for the system tests.

comment:63 Changed 7 years ago by Keith Brown

The header skipping function from this ticket seems to need more work as it's causing LoadLotsOfFiles to fail (again), but this time legitimately as the file Russell added DIRECTM1_15785_12m_31Oct12_v12.dat is a legit file that should be loaded, but the loader is rejecting it as the header info looks a bit like data.

Opened new ticket #8509 to fix it.

comment:64 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 8577

Note: See TracTickets for help on using tickets.