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

Fix doctests and let TravisCI run doctests #811

Merged
merged 75 commits into from Nov 18, 2013

Conversation

ahojnnes
Copy link
Member

@ahojnnes ahojnnes commented Nov 3, 2013

No description provided.

@cdeil
Copy link
Contributor

cdeil commented Nov 3, 2013

Thanks for cleaning all this up!

Could you additionally build the html docs on travis-ci?

@ahojnnes
Copy link
Member Author

ahojnnes commented Nov 3, 2013

I am not able to get it working. I cannot find a way to ignore the skimage.viewer test cases with --ignore-files or __test__ = False nor can I get a working env of PyQt4 on Travis.

Does anybody have an idea to fix this?

@jni
Copy link
Member

jni commented Nov 3, 2013

@ahojnnes, does the skipif decorator not help here?

I can't find the failed line in Travis so I don't know what else to look for...

@ahojnnes
Copy link
Member Author

ahojnnes commented Nov 3, 2013

The problem are not the normal tests but to the nosetests to skip the doctests in skimage.viewer. The failed line is the last line of all tests at the moment (because it "cannot connect to X server").

@cdeil
Copy link
Contributor

cdeil commented Nov 3, 2013

Can you use # doctest: +SKIP to skip the doctests that don't work on travis-ci?

@cdeil
Copy link
Contributor

cdeil commented Nov 3, 2013

I have a local Ubuntu virtual box where I'm trying to get it to work.
So far I'm seeing the same errors as you get on travis-ci though and don't know how to get it to work.
If there's anything you'd like me to try let me know.

@ahojnnes
Copy link
Member Author

@stefanv @jni I made some more improvements, see skimage.test and skimage.doctest. I hope everything is good now.

@ahojnnes
Copy link
Member Author

Regarding the runtime of the whole travis process:

Before Py2 ~16min, Py3 ~21min
After Py2 ~10min, Py3 ~15min

"""

# window extent in one direction
wext = (window_size - 1) / 2
wext = int((window_size - 1) / 2)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be (window_size - 1) // 2?

@stefanv
Copy link
Member

stefanv commented Nov 18, 2013

This is great, thanks @ahojnnes . Can I go ahead and merge?

@ahojnnes
Copy link
Member Author

Nothing holding this back from merging from my side.

stefanv added a commit that referenced this pull request Nov 18, 2013
Fix doctests and let TravisCI run doctests
@stefanv stefanv merged commit b00e181 into scikit-image:master Nov 18, 2013
@stefanv
Copy link
Member

stefanv commented Nov 18, 2013

Good stuff. These doctest fixes should also be part of the next bugfix release?

@ahojnnes
Copy link
Member Author

Yes, I would include them in the next bugfix release. Before we push it, we should maybe let Christoph Deil and others run the tests quickly to confirm everything works correctly?

@cdeil
Copy link
Contributor

cdeil commented Nov 18, 2013

Works for me on Mac, except for these two doctest fails on Python 3.3:
https://gist.github.com/cdeil/7534461

Do you need to click the "Restart build" button on travis-ci for open pull requests to benefit from this improved testing or did that happen automatically?

@stefanv
Copy link
Member

stefanv commented Nov 19, 2013

@cdeil, which version of Qt do you have?

@cdeil
Copy link
Contributor

cdeil commented Nov 19, 2013

I think the problem is that I didn't have PyQT or PySide installed for Python 3.3.

So I guess what is missing in that doctest is some marker to skip it if Qt is not available instead of reporting a test error, because it's an optional skimage dependency?

@stefanv
Copy link
Member

stefanv commented Nov 19, 2013

I have no idea how to do that in a doctest. @ahojnnes?

@ahojnnes
Copy link
Member Author

I think there is no direct way to achieve this inside the doc tests. But you could ignore the viewer module if skimage.viewer.qt.qt_api is None?

@JDWarner
Copy link
Contributor

This PR from NiPy might be of interest - it includes a skip decorator for a given condition.

dipy/dipy#105

@ahojnnes
Copy link
Member Author

I don't have time I look into it but from a quick glimpse it seems to solve the problem.

+1 for integrating this if anybody ha enough time for this.

@stefanv
Copy link
Member

stefanv commented Nov 20, 2013

Looks good to me.

@stefanv
Copy link
Member

stefanv commented Nov 20, 2013

@cdeil @JDWarner Do either of you have time to make a PR for this?

@JDWarner
Copy link
Contributor

Addition incoming. The DiPy framework needed to be tweaked a bit for it to work as a decorator for both function and class docstrings, as classes don't expose a .__globals__ attribute.

@stefanv
Copy link
Member

stefanv commented Nov 20, 2013

I see a new DiPy contributor in the works...double PR!

@ahojnnes ahojnnes deleted the doctests branch November 25, 2013 22:42
JDWarner pushed a commit that referenced this pull request May 4, 2020
* Adding first draft of Cython relabel_sequential
Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

* fixed order of args for map_array

* Add ArrayMap str and repr

* Add __call__ method to ArrayMap object

* Add missing colon

* Add array representation for ArrayMap

* Use array representation to preserve relabel_sequential doctests

* Restore ValueError checks for relabel_sequential

* Fix PEP8 issues

* Ensure 0 always maps to 0

* Ensure dtype is correct for output

* Fix incorrect dtype for array being relabeled

The docs specify that only integer arrays are supported. This change
was added in gh-811 without any discussion so imho safe to revert.

* Fix dtype of output and map

* Inverse map dtype should actually map the input dtype

* Fix docstring for map_array, check for contiguous out

* Add docstring for ArrayMap

* Rename attributes in ArrayMap

* Add C++0x standard to the compiler options

* added missing paren in docstring

* mention large size of returned array in __array__ docstr

* adjusted sample array to fix doctest

Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

* added tests for dtype overflow

* test relabel_seq... works for large input

* Simplify checking of contiguous array

Co-Authored-By: Riadh Fezzani <rfezzani@gmail.com>

* Minor code updates from review

Co-Authored-By: Riadh Fezzani <rfezzani@gmail.com>

* Restore reshaping of output array when provided

* Add note about -std=c++0x flag

* Ensure reshape is zero-copy

* Clarify error message when output has incorrect shape

* Remove unused numpy import

* Update relabel_sequential docstring to indicate new output types

* Raise error if the input array has non-integer type

* Ensure arrays being mapped and value arrays have same dtype

* Add doctests for ArrayMap.__str__ and fix

* Rework logic for in-place reshaping

* Add test for error when labels dtype is not int

* Add test for incorrect map_array output shape

* Test discontiguous output array

* Add test for long reprs for ArrayMap

* Add test for calling ArrayMap

* Remove long-deprecated relabel_from_one

* Add len to ArrayMap

* Add support for setitem and scalar mapping

* Add support for slicing and updating map in-place

* Allow bool indices in arraymap getitem

Co-authored-by: Volker Hilsenstein <volker.hilsenstein@monash.edu>
Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>
Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>
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.

None yet

5 participants