get_data consistence

[skoudoro]

The goal of this PR is to resolve an old issue #10 .

Can you have a look @matthew-brett @arokem ?

[arokem]

This looks good to me.

@matthew-brett made the comment that it would be preferable to return data, but I think that returning file-names makes sense. It's more transparent for users to see how they would adapt this to their own data, when the file-names are explicitly mentioned.

But maybe @matthew-brett has reconsidered this since 2011? 😄

[coveralls]

Coverage decreased (-0.01%) to 85.405% when pulling 2c828ee on skoudoro:issue_10_get_data_consistence into 5136340 on nipy:master.

[coveralls]

Coverage decreased (-0.01%) to 85.405% when pulling 2c828ee on skoudoro:issue_10_get_data_consistence into 5136340 on nipy:master.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@5136340). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1283   +/-   ##
=========================================
  Coverage          ?   87.09%           
=========================================
  Files             ?      228           
  Lines             ?    28749           
  Branches          ?     3088           
=========================================
  Hits              ?    25040           
  Misses            ?     3004           
  Partials          ?      705

Impacted Files	Coverage Δ
dipy/sims/tests/test_phantom.py	94.87% <100%> (ø)
dipy/reconst/tests/test_sfm.py	98.19% <100%> (ø)
dipy/data/__init__.py	92.15% <100%> (ø)
dipy/reconst/tests/test_csdeconv.py	99.36% <100%> (ø)
dipy/core/tests/test_gradients.py	98.11% <100%> (ø)
dipy/direction/tests/test_peaks.py	99.44% <100%> (ø)
dipy/tracking/tests/test_life.py	100% <100%> (ø)

---

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 5136340...2c828ee. Read the comment docs.

[matthew-brett]

This looks better to me - thanks. I think it's fine to return files, just as long as it's obvious from the docstrings / variable names that that is the case. For example, how about get_data_fnames rather than get_data ? Would that help with backwards compatibility?

[skoudoro]

Explicit better than implicit (like you said @arokem ) so I agree with @matthew-brett. I prefer get_data_fnamesthan get_data. I made this mistake the first time because I was not expecting filenames.

@Garyfallidis, any thoughts ?

If it is ok for all of you, I can do a new PR with this change.

[arokem]

You can make that change on this PR in a follow-up commit

[Garyfallidis]

get_fnames or get_data_fnames is fine for me. When refactoring this function take your time it is used in many many places.

[arokem]

Thinking about this a bit more, I prefer `get_fnames`. It's already in the `dipy.data` namespace.

…

On Mon, Jul 3, 2017 at 1:11 PM, Eleftherios Garyfallidis < ***@***.***> wrote: get_fnames or get_data_fnames is fine for me. When refactoring this function take your time it is used in many many places. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1283 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNrouMpsFHOCLws2C3wVtAV9pqyHeks5sKUrkgaJpZM4OIeLS> .

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@756b519). Click here to learn what that means.
The diff coverage is 98.22%.

@@            Coverage Diff            @@
##             master    #1283   +/-   ##
=========================================
  Coverage          ?   87.63%           
=========================================
  Files             ?      248           
  Lines             ?    34172           
  Branches          ?     3748           
=========================================
  Hits              ?    29945           
  Misses            ?     3338           
  Partials          ?      889

Impacted Files	Coverage Δ
dipy/tracking/eudx.py	94.11% <ø> (ø)
dipy/tests/test_scripts.py	100% <ø> (ø)
dipy/align/reslice.py	97.29% <ø> (ø)
dipy/tracking/tests/test_metrics.py	100% <ø> (ø)
dipy/segment/clustering.py	94.63% <ø> (ø)
dipy/sims/voxel.py	90.58% <ø> (ø)
dipy/reconst/shore.py	85.15% <ø> (ø)
dipy/workflows/tests/test_masking.py	91.66% <100%> (ø)
dipy/align/tests/test_streamlinear.py	97.74% <100%> (ø)
dipy/align/tests/test_imwarp.py	99.22% <100%> (ø)
... and 41 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 756b519...bb173d2. Read the comment docs.

[skoudoro]

ok, I made all the update and replace get_data by get_fnames.

This old PR is ready to go @arokem @Garyfallidis @matthew-brett. Can you check it?

Thank you

[skoudoro]

Any thought concerning this PR?

[arokem]

LGTM. Let's merge it tomorrow, unless there are any objections.

[skoudoro]

Thanks!