Skip to content

make tests pass on Windows (64 bit)#197

Closed
flothesof wants to merge 4 commits intoToFuProject:develfrom
flothesof:issue95_windows_compatibility
Closed

make tests pass on Windows (64 bit)#197
flothesof wants to merge 4 commits intoToFuProject:develfrom
flothesof:issue95_windows_compatibility

Conversation

@flothesof
Copy link
Copy Markdown
Contributor

Hi @lasofivec @Didou09
I've just successfully patched my branch from current devel so that tests pass on Windows (64 bit).
This is a follow-up to what we discussed earlier (replacing long by longlong in the Cython source) but simpler: I basically just had to patch a couple of variable initializations and tests. Could you check this out and let me know what you think?
On my machine, all 118 tests pass with this.

@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Sep 16, 2019

Hello @flothesof! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 49:80: E501 line too long (99 > 79 characters)
Line 61:80: E501 line too long (85 > 79 characters)
Line 91:80: E501 line too long (132 > 79 characters)

Comment last updated at 2019-09-23 16:06:56 UTC

adding spaces conforming pep8 rules
Adding spaces according to pep8 rules
@lasofivec
Copy link
Copy Markdown
Collaborator

It looks like it's failing with python 2:

=====================================================================
ERROR: tofu.tests.tests02_data.tests03_core.Test01_DataCam12D.test10_dtreat_set_interp_indch
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/miniconda/lib/python2.7/site-packages/tofu/tests/tests02_data/tests03_core.py", line 333, in test10_dtreat_set_interp_indch
    oo.set_dtreat_interp_indch( ind )
  File "/home/travis/miniconda/lib/python2.7/site-packages/tofu/data/_core.py", line 1065, in set_dtreat_interp_indch
    indch = _format_ind(indch, n=self._ddataRef['nch'])
  File "/home/travis/miniconda/lib/python2.7/site-packages/tofu/data/_core.py", line 71, in _format_ind
    raise Exception(msg)
Exception: Index must be a int, or an iterable of bool or int !
======================================================================
ERROR: tofu.tests.tests02_data.tests03_core.Test02_DataCam12DSpectral.test10_dtreat_set_interp_indch
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/miniconda/lib/python2.7/site-packages/tofu/tests/tests02_data/tests03_core.py", line 333, in test10_dtreat_set_interp_indch
    oo.set_dtreat_interp_indch( ind )
  File "/home/travis/miniconda/lib/python2.7/site-packages/tofu/data/_core.py", line 1065, in set_dtreat_interp_indch
    indch = _format_ind(indch, n=self._ddataRef['nch'])
  File "/home/travis/miniconda/lib/python2.7/site-packages/tofu/data/_core.py", line 71, in _format_ind
    raise Exception(msg)
Exception: Index must be a int, or an iterable of bool or int !

@flothesof
Copy link
Copy Markdown
Contributor Author

I'll try the modifications under Python 2 then. Do we really have to support Python 2 (see https://pythonclock.org/) ?

@lasofivec
Copy link
Copy Markdown
Collaborator

I know :(
but I am afraid for the moment we haven't decided to stop supporting python 2 as in some machines I think it's still the default python...
@Didou09 will confirm (or deny) this...

@flothesof
Copy link
Copy Markdown
Contributor Author

Hi @lasofivec
I've just spent a good amount of time getting tofu to run on Windows with Python 2.7.
I didn't succeed.
The Travis log is obscure to me, so I'm going to add a better debug message. Hopefully, this will allow us to progress.

@lasofivec
Copy link
Copy Markdown
Collaborator

Thanks for looking into it, I can also check it out, any hint on what you think it is ?
Just from the error:

Exception: Index must be a int, or an iterable of bool or int !

raised from

n=self._ddataRef['nch']

looks like self._ddataRef is supposed to be a dictionary and somehow python thinks it is an array?
I'm going away on holidays from Thursday, so I am adding this to my TODO for tomorrow.

@flothesof
Copy link
Copy Markdown
Contributor Author

I've just stepped through the unit test on Python 3. As far as I understand, test test10_dtreat_set_interp_indch tries to activate channels 0, 10 and 20 on a 30 channel DataCam1D. The test fails at the point when the list of channels is being converted to a boolean array of size 30 with the indices corresponding to 0, 10 and 20 set to True (the rest to False).

As far as I understand, the error means that the first type in [ 0 10 20] is not an integer type from the list of possible integers types (and is not related to self._ddataRef in my opinion).

As a side note:

  • the docstring for _format_ind is not helpful
  • the error message is also not helpful (it would be better if it printed the dtype that caused the problem)

I'm trying to write commit to improve these.

@lasofivec
Copy link
Copy Markdown
Collaborator

Ok I'll be testing from b2c2894
More news this afternoon

@flothesof
Copy link
Copy Markdown
Contributor Author

Changing the error message was helpful: the latest travis build shows this on Python 2.7.

Exception: Index must be int, or an iterable of bool or int (first element of index has type: <type 'numpy.int64'>)!

So it seems that we have to add int64 to the list?

@flothesof
Copy link
Copy Markdown
Contributor Author

After reading the source code more, I think the way the type is checked is weird.
In the build, it seems that type(ind[0]) in [int, np.int64, np.int32, np.long] returns True under Py36 but False under Py27. A better way to check this might be to convert the array to a numpy format rightaway and then inspect the dtype instead of the type?

Copy link
Copy Markdown
Collaborator

@lasofivec lasofivec left a comment

Choose a reason for hiding this comment

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

I agree with everything, we have to see this strange bug.
Just one suggestion for future PR.
We are trying to introduce only PEP8-conforming code, could you please check that your PR passes auto-flake8 test next time ? 🙏 (since your coding style is really good it's just the 80 character rule)

Comment thread tofu/data/_core.py
>>> _format_ind(ind=[0, 3], n=4)
[True, False, False, True]

"""
Copy link
Copy Markdown
Collaborator

@lasofivec lasofivec Sep 24, 2019

Choose a reason for hiding this comment

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

👏 👍
Thanks for the doc.

@lasofivec
Copy link
Copy Markdown
Collaborator

After reading the source code more, I think the way the type is checked is weird.
In the build, it seems that type(ind[0]) in [int, np.int64, np.int32, np.long] returns True under Py36 but False under Py27. A better way to check this might be to convert the array to a numpy format rightaway and then inspect the dtype instead of the type?

I'm confused...
print(type(ind[0]), lInt[1], type(ind[0]) == lInt[1])

returns

(<type 'numpy.int64'>, <type 'numpy.int64'>, False)

how is this possible?

@lasofivec
Copy link
Copy Markdown
Collaborator

Changing the error message was helpful: the latest travis build shows this on Python 2.7.

Exception: Index must be int, or an iterable of bool or int (first element of index has type: <type 'numpy.int64'>)!

So it seems that we have to add int64 to the list?

The type is already on the list and it's still failing...

@flothesof
Copy link
Copy Markdown
Contributor Author

I'm going to close this pull request since it is superseded by #199

@flothesof flothesof closed this Sep 25, 2019
@flothesof flothesof deleted the issue95_windows_compatibility branch September 25, 2019 11:30
@Didou09 Didou09 mentioned this pull request Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants