Fix for ARFF reader regression (#10232)

[vnmabus]

• Quoted nominal values are now properly unquoted.
• A regression test has been added.

[rgommers]

Thanks @vnmabus! @sebp can you confirm that this solves your issue?

[vnmabus]

It seems that for some reasons the test fails on some versions and works in others. I am not sure why.

[sebp]

I can confirm that it does solve the issue for me. Strictly speaking, stripping quotes leads to different values being returned compared to versions prior to 1.3, but most of the time it would make sense to do this.

I noticed that your code accounts for situations where the first or last character is a quote, but not both. I strongly suggest to add a test case for this too.

[sebp]

I found a slightly esoteric setup hat still results in an exception:

@attribute age numeric
@attribute smoker {'  yes', 'no  '}
@data
18,'no  '
24,'  yes'
44,'no  '
56,'no  '
89,'  yes'
11,'no  '

also results in ValueError: no value not in (' yes', 'no ').

How about using csv.reader to parse the list of attributes too?

[sebp]

Thanks for the changes @vnmabus, everything works as expected on my end.

[rgommers]

The one CI failure on Azure is real. A bit puzzling though, 'no == 'no'` failing ....

________________ ERROR at setup of TestQuotedNominal.test_data ________________
[gw0] win32 -- Python 3.5.4 C:\hostedtoolcache\windows\Python\3.5.4\x64\python.exe

self = <scipy.io.arff.tests.test_arffread.TestQuotedNominal object at 0x0000022EAB6C76A0>

    def setup_method(self):
>       self.data, self.meta = loadarff(test_quoted_nominal)

self       = <scipy.io.arff.tests.test_arffread.TestQuotedNominal object at 0x0000022EAB6C76A0>

C:\hostedtoolcache\windows\Python\3.5.4\x64\lib\site-packages\scipy\io\arff\tests\test_arffread.py:342: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
C:\hostedtoolcache\windows\Python\3.5.4\x64\lib\site-packages\scipy\io\arff\arffread.py:738: in loadarff
    return _loadarff(ofile)
C:\hostedtoolcache\windows\Python\3.5.4\x64\lib\site-packages\scipy\io\arff\arffread.py:803: in _loadarff
    a = list(generator(ofile))
C:\hostedtoolcache\windows\Python\3.5.4\x64\lib\site-packages\scipy\io\arff\arffread.py:801: in generator
    yield tuple([attr[i].parse_data(row[i]) for i in elems])
C:\hostedtoolcache\windows\Python\3.5.4\x64\lib\site-packages\scipy\io\arff\arffread.py:801: in <listcomp>
    yield tuple([attr[i].parse_data(row[i]) for i in elems])
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <scipy.io.arff.arffread.NominalAttribute object at 0x0000022EB4DA74E0>
data_str = "'no'"

    def parse_data(self, data_str):
        """
        Parse a value of this type.
        """
        if data_str in self.values:
            return data_str
        elif data_str == '?':
            return data_str
        else:
            raise ValueError("%s value not in %s" % (str(data_str),
>                                                    str(self.values)))
E           ValueError: 'no' value not in ('yes', 'no')

data_str   = "'no'"
self       = <scipy.io.arff.arffread.NominalAttribute object at 0x0000022EB4DA74E0>

C:\hostedtoolcache\windows\Python\3.5.4\x64\lib\site-packages\scipy\io\arff\arffread.py:165: ValueError

[vnmabus]

So, in some environments, the data do not have the quotes stripped but the attribute values do, in spite of using the same function for reading them. Any suggestion on which difference is the important one between the affected versions, and how can I test this error locally? 😅

[vnmabus]

Ok, it seems to be that some versions do not have this patch applied:
python/cpython@2411292
How should I proceed now? Should I provide a workaround?

[rgommers]

Nice catch. That patch is pretty recent it looks like, so there'll be quite some users that won't have it. If a workaround isn't too hard, then yes that sounds good.

[rgommers]

Nice, that fixed it!

One more minor CI issue:

39.88s$ pycodestyle scipy benchmarks/benchmarks
scipy/io/arff/arffread.py:458:112: E502 the backslash is redundant between brackets
1       E502 the backslash is redundant between brackets

[rgommers]

All green now, merged. Thanks @vnmabus for the quick fix, and @sebp for reporting!

[vnmabus]

Sorry for bothering you @rgommers but I am not sure if these commits should be also pushed to master. I opened this PR against the maintenance branch, and I am realizing now that maybe that was not right.

[rgommers]

Oh thanks for pointing that out, I totally missed that. I'll forward-port them.

[pv]

@vnmabus: For future reference, as a general rule everything should go to master first.