search

wtclarke / fsl_mrs

Mirror of the FSL-MRS gitlab repository
https://git.fmrib.ox.ac.uk/fsl/fsl_mrs
Other
14 stars 8 forks source link

Fix parse_metab_groups behaviour #4

Closed alexcraven closed 3 years ago

alexcraven commented 3 years ago

parse_metab_groups failed for lists containing a single entry with '+' notation

wtclarke commented 3 years ago

Hi @alexcraven thanks for this bug report. Could you just clarify what situations fail? I think you are saying that e.g. 'NAA+' fails but that 'NAA+NAAG' works. Is that right?

alexcraven commented 3 years ago

Not exactly, the situation was such that:

--metab_groups NAA+NAAG Glu+Gln would work as expected, but --metab_groups Glu+Gln would fail.

Ie, a list of multiple groups (possibly containing '+') would be parsed correctly, but a single group would not.

alexcraven commented 3 years ago

...so my fix simply replicates the same logic from the list-of-groups case, and applies it to the single-group case (with any unmentioned metabolites still falling into default group 0).

wtclarke commented 3 years ago

Great, that makes sense and fails for me as well. I'll include this and add some tests as well. I might not do that via this pull request mechanism as a) I forget exactly how the mirroring works between our internal gitlab and github and b) our gitlab server fell over this morning.

There are a number of small fixes that have been implemented recently so I will generate a new minor version including this fix once I can.

alexcraven commented 3 years ago

Perfect! Thanks. Is it easier if I just send future things by email instead?

wtclarke commented 3 years ago

Here is fine, I think I can still make the merge via gitlab (when it is back working) and will then mark as closed manually.