smeerten / ssnake

A program for the analysis of NMR data.
Other
21 stars 15 forks source link

Inserting workspace with incorrect dimensions causes error and badly modified data #142

Closed Famlam closed 1 year ago

Famlam commented 1 year ago
  1. Have spectrum A of size 15000x12 and spectrum B of size 15000x1 (or any size, as long as A is a NxM matrix and B an Nx1 matrix; N,M>1)
  2. On spectrum A, use Combine -> Insert From Workspace. Select spectrum B in the dialog that pops up

The following error is shown. Note that, despite the error being shown and the appearance nothing happened, the data is modified (badly). Even worse, the undo action is not yet set when the error occurs.

Traceback (most recent call last): 
File "widgetClasses.py", line 407, in applyAndClose self.applyFunc() 
File "ssNake.py", line 5163, in applyFunc self.father.current.insert(self.father.father.workspaces[ws].masterData.getData(), pos) 
File "views.py", line 1772, in insert self.data.insert(data, pos, self.axes[-1]) 
File "spectrum.py", line 382, in insert self.addHistory("Inserted " + str(data.shape()[axis]) + " datapoints in dimension " + str(axis + 1) + " at position " + str(pos)) 
IndexError: tuple index out of range 

This PR throws a (human-understandable) error and prevents data from being modified if the dimensions are incorrect.

Famlam commented 1 year ago

p.s.: note for reproducing that the dimension with 15000 points in the example should be the active dimension of spectrum A (and also for B since that only has 1 dimension)

An alternative fix would be to set the undo action directly after modifying the data, so that the user can revert to the previous state, but I prefer to just throw a human readable error before modifying the data.

wfranssen commented 1 year ago

I cannot reproduce your error. For me, a message is given that the two data sets are not of the same size, and nothing happens.

In my case I use Combine -> Combine Workspaces, and both spectrum A and B an the right side (as 'to combine').

Not sure what I do differently than you....

Famlam commented 1 year ago

I made a stupid mistake in my first post.... I wrote 'Combine workspace', but I should've written Insert From Workspace, the option underneath. (I somehow always mix them up, hence...). Updated that post too. Apologies for this.

Attached is some dummy data that you can use. Just load both json files in ssNake, then follow the steps above.

Just to be sure, this is how it should look just before getting the error (=when you press OK) image

Famlam commented 1 year ago

Can you reproduce the issue with this @wfranssen ?

wfranssen commented 1 year ago

Yes, I can now confirm this. This problem is part of Issue #98 "Combine actions do not check broadcasting". Your pull request probably does not cover all problems that can occur for 'Insert', but it is a good start.

I will merge it now. Thanks for the work.

Famlam commented 1 year ago

Your pull request probably does not cover all problems that can occur for 'Insert', but it is a good start.

Correct, if you for instance invert spectrum A and B in this PR, an error also pops up. However, in that case the error occurs before data is modified, so that error is "harmless" (and, simultaneously, more difficult to fix).

Thanks for merging!