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

get_data consistence #1283

Merged
merged 5 commits into from Nov 28, 2018

Conversation

Projects
None yet
6 participants
@skoudoro
Copy link
Member

skoudoro commented Jun 28, 2017

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

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

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jun 28, 2017

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

This comment has been minimized.

Copy link

coveralls commented Jun 28, 2017

Coverage Status

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

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 28, 2017

Coverage Status

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

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 29, 2017

Codecov Report

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

Impacted file tree graph

@@            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 comment has been minimized.

Copy link
Member

matthew-brett commented Jun 29, 2017

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

This comment has been minimized.

Copy link
Member

skoudoro commented Jun 29, 2017

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

This comment has been minimized.

Copy link
Member

arokem commented Jul 3, 2017

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

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jul 3, 2017

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

This comment has been minimized.

Copy link
Member

arokem commented Jul 3, 2017

@skoudoro skoudoro changed the title get_data consistence [WIP] get_data consistence Jul 5, 2017

@skoudoro skoudoro force-pushed the skoudoro:issue_10_get_data_consistence branch from 2c828ee to ccadf10 Nov 16, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 16, 2018

Codecov Report

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

Impacted file tree graph

@@            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 skoudoro changed the title [WIP] get_data consistence get_data consistence Nov 16, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Nov 16, 2018

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

This comment has been minimized.

Copy link
Member

skoudoro commented Nov 28, 2018

Any thought concerning this PR?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Nov 28, 2018

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

@arokem arokem merged commit 12e1447 into nipy:master Nov 28, 2018

4 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Nov 28, 2018

Thanks!

@skoudoro skoudoro deleted the skoudoro:issue_10_get_data_consistence branch Nov 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment