PuffinPlot issue tracker

Bug: a88/258

ID : 2588eaed-fd41-4e3b-a0e2-ab34e2769c3c
Short name : a88/258
Status : fixed
Severity : minor
Assigned :
Reporter : Pontus Lurcock <pont@talvi.net>
Creator : Pontus Lurcock <pont@talvi.net>
Created : Sat, 01 Dec 2018 10:12:11 +0000
Target : 1.4
Summary : Crash when ASC imported into unoriented samples

Comment: --------- Comment ---------
ID: 8ecdb967-ff84-4dee-8278-730b39cc994f
Short name: a88/258/8ec
From: Pontus Lurcock <pont@talvi.net>
Date: Sat, 01 Dec 2018 10:33:28 +0000

Report from Kuo-Hang Chen, 2018-11-27. Steps to reproduce:

1. Read a file using a custom format with no sample or formation
   orientation. All correction orientations/parameters will be NaN.

2. Import AMS data from an ASC file with one or more sample names
   that match sample names already in the suite.

Result: PuffinPlot crashes in the Tensor constructor, which throws an
IllegalArgumentException when it finds that the correction matrices
contain NaNs -- because they have been constructed from the sample
and formation orientations, which were all left as NaN during the
initial custom file import.

Desired behaviour:

1. Read a file using a custom format with no sample or formation
   orientation. File importer sets reasonable default values for
   the orientation parameters (the usual 0/90 and 0/0).

2. Import AMS data from an ASC file with one or more sample names
   that match sample names already in the suite. AMS importer asks
   whether to overwrite existing orientation parameters with
   values from the ASC file and acts accordingly.

A simpler fix would also be possible: retain the behaviour of leaving
the corrections as NaN, and have the ASC importer overwrite them
automatically in this case. However, I feel that having NaN corrections
is in itself a bug, even if this is the only crash that it's so far led
to. The correction parameters are so widely used that it's safer to have
them well-defined from the start. Graceful handling of NaN values would
in any case probably just consist of falling back to the same sensible
defaults, so it makes more sense to set them explicitly from the start.


Comment: --------- Comment ---------
ID: 5ad1fa30-3105-4062-affc-578323acb0e2
Short name: a88/258/5ad
From: Pontus Lurcock <pont@talvi.net>
Date: Sat, 04 May 2019 07:21:09 +0000

Now fixed as specified; while evaluating the fix, the bug reporter
found that AMS orientations were incorrect for another ASC file
of his. I discovered that this was because the file didn't use the
same orientation conventions as PuffinPlot. PuffinPlot now reads
the orientation parameters from the ASC file and automatically
converts the tensor and sample and formation corrections into its
own system. Tested successfully with the offending file, and I've
added a truncated version of it to the test suite. Waiting for
confirmation from the bug reporter before closing the ticket.


Comment: --------- Comment ---------
ID: 0e1ed27a-0456-412a-9df5-c63ef42883d4
Short name: a88/258/0e1
From: Pontus Lurcock <pont@talvi.net>
Date: Tue, 07 May 2019 18:13:42 +0000

Kuo-Hang confirms that bugfix works for him, so I'm closing this
ticket.