smeerten / ssnake

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

Prevent saving as invalid matlab variable name #128

Closed Famlam closed 1 year ago

Famlam commented 1 year ago

Matlab variable names must start with a letter, then either of letters, numbers or _ (https://www.mathworks.com/help/matlab/matlab_prog/variable-names.html) If the spectrum name doesn't satisfy this (e.g. a spectrum called "01_mysample_1Hecho"), convert it to something that can actually be read by Matlab. (Prefixing `nmrto ensure the start with a letter, and replacing all invalid chars by_`)

Technically, it should also ensure the spectrum name isn't something like if, for, end, ..., but let's ignore that for now as the likelyhood of that is small

jtrebosc commented 1 year ago

Shoudn't this be handled in the Matlab save dialog box ? Changing the name in specIO will be done silently and the user may not realise it. Julien

Famlam commented 1 year ago

The save dialog box doesn't handle the variable name, only the file name (e.g. the thing ending in .mat)

jtrebosc commented 1 year ago

In a first approach, one should set the default filename in conformation with your prescriptions: nmr prefix and white spaces replaced with . Now the proposed filename is only the workspace name. In a mor complex handling, after the QFileDialog returns a filename, one could validate it, amend it if necessary and after a warning message open a new QFileDialog with the amended filename for confirmation by user. Best, Julien

Famlam commented 1 year ago

As mentioned, the filename doesn't matter, that can be anything, it's about the variable name inside the file which has restrictions.

My patch prevents generating invalid matlab files; adding optional UI I'll leave for the developers. Right now you get a broken file, after this patch you get a working file with a matlab-valid similar-named variable

jtrebosc commented 1 year ago

OK, I got it. Thanks for the explanation. I don't master enough ssnake yet (and even less matlab format) to see quickly if this could impact some other part of ssnake. Maybe Bas or Wouter with accept the pull request shortly. Otherwise I will test it and accept as it doesn't seem to have large impact and I have a big merge in preparation on Topspin/XwinNMR data import.

smeerten commented 1 year ago

The loading of data with Matlab with variables starting with a number has indeed been issue before. And since ssNake does not care about the variable name, this is a nice solution. I'll merge this pull request.