Skip to content

Clarified error messages in LazyTractogram #504

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

Merged
merged 2 commits into from
Mar 25, 2017

Conversation

MarcCote
Copy link
Contributor

This PR clarifies what "generator function" means in relevant error messages. It also makes sure all values contained in a LazyDict object are callable.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0006%) to 95.995% when pulling 3d64f95 on MarcCote:doc_lazy_tractogram into 1d27aef on nipy:master.

@codecov-io
Copy link

codecov-io commented Jan 19, 2017

Codecov Report

Merging #504 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
+ Coverage   94.02%   94.05%   +0.02%     
==========================================
  Files         166      166              
  Lines       21843    22047     +204     
  Branches     2325     2345      +20     
==========================================
+ Hits        20538    20736     +198     
- Misses        875      877       +2     
- Partials      430      434       +4
Impacted Files Coverage Δ
nibabel/streamlines/tests/test_tractogram.py 97.67% <100%> (-1.32%)
nibabel/streamlines/tractogram.py 96.67% <100%> (+0.38%)
nibabel/viewers.py 96.66% <0%> (-0.67%)
nibabel/streamlines/tests/test_trk.py 100% <0%> (ø)
nibabel/info.py 100% <0%> (ø)
nibabel/tests/test_parrec.py 100% <0%> (ø)
nibabel/tests/test_nifti1.py 98.32% <0%> (ø)
nibabel/streamlines/tests/test_array_sequence.py 99.49% <0%> (+0.03%)
nibabel/streamlines/array_sequence.py 98.91% <0%> (+0.04%)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d27aef...531842e. Read the comment docs.

Copy link
Member

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

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

Suggestion for rephrase.

raise TypeError("`value` must be a generator function or None.")
if not callable(value):
msg = ("Values in a `LazyDict` must be generator functions."
" Those are functions which whenever are called return an"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "These are functions which, when called, return an instantiated generator."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank for the suggestion, it is indeed way better.

@@ -604,7 +607,10 @@ def _apply_affine():

def _set_streamlines(self, value):
if value is not None and not callable(value):
raise TypeError("`streamlines` must be a generator function.")
msg = ("`streamlines` must be a generator function. That is a"
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@MarcCote
Copy link
Contributor Author

Completely forgot about this PR :O.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 96.022% when pulling 531842e on MarcCote:doc_lazy_tractogram into 1d27aef on nipy:master.

@MarcCote
Copy link
Contributor Author

@matthew-brett This PR should be good to go. I've modified some outputs messages as you suggested.

@matthew-brett
Copy link
Member

Thanks - in it goes.

@matthew-brett matthew-brett merged commit 8de91a1 into nipy:master Mar 25, 2017
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.

4 participants