Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG/TST: io: fix redundant if-condition in arffread.py #11888

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

distroinfinity
Copy link
Contributor

Reference issue

#11864

What does this implement/fix?

Tried to improve the elif "yy" condition

Additional information

@miladsade96 miladsade96 added maintenance Items related to regular maintenance tasks scipy.io labels Apr 19, 2020
'31-10-15'
], dtype='datetime64[Y]')

assert_array_equal(self.data["attr_yeari"], expected)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some debug prints I see:

__________________________________________________________________________________________________________ TestDateAttribute.test_yeari_attribute __________________________________________________________________________________________________________
TypeError: invalid type promotion

The above exception was the direct cause of the following exception:
scipy/io/arff/tests/test_arffread.py:219: in test_yeari_attribute
    assert_array_equal(self.data["attr_yeari"], expected)
E   DeprecationWarning: elementwise comparison failed; this will raise an error in the future.
------------------------------------------------------------------------------------------------------------------- Captured stdout call -------------------------------------------------------------------------------------------------------------------
actual: ['1999-01-31' '2004-12-01' '2017-04-28' '2000-09-10' '2013-11-30'
 '2031-10-15']
expected: ['0099' '0004' '0017' '0000' '0013' '0031']
datetime64[D] datetime64[Y]

'00-09-10',
'13-11-30',
'31-10-15'
], dtype='datetime64[Y]')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are "years" the smallest units you're expecting to get back here? If I switch to days it is a little closer:

actual: ['1999-01-31' '2004-12-01' '2017-04-28' '2000-09-10' '2013-11-30'
 '2031-10-15']
expected: ['0099-01-31' '0004-12-01' '0017-04-28' '0000-09-10' '0013-11-30'
 '0031-10-15']
datetime64[D] datetime64[D]

Looks like 31 is getting converted to 2031, but I suppose we're expecting only the two-digit year to be extracted unambiguously here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be what happens under the hood:

>>> datetime.datetime.strptime('31-10-15', '%y-%m-%d')
datetime.datetime(2031, 10, 15, 0, 0)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong, but the prospects for returning a datetime object with the century somehow stripped out are maybe not great cc @eric-wieser ?

The question of what we expect to get back here in SciPy is actually not immediately clear to me, but perhaps it would actually match the century restoration behavior of the std lib?

@lucascolley lucascolley added the defect A clear bug or issue that prevents SciPy from being installed or used as expected label Mar 14, 2024
@lucascolley lucascolley changed the title WIP, arffread.py -- redundant if-condition BUG/TST: io: fix redundant if-condition in arffread.py Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected maintenance Items related to regular maintenance tasks scipy.io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants