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

RF - dipy.tracking.local #1926

Merged
merged 14 commits into from Jul 31, 2019

Conversation

@gabknight
Copy link
Contributor

commented Jul 27, 2019

Addresses in part #1501

  • move the module dipy.tracking.local into dipy.tracking
  • renamed dipy.tracking.localtracking.py to dipy.tracking.local_tracking.py
  • renamed dipy.tracking.tissue_classifier.TissueClass to dipy.tracking.tissue_classifier.StreamStatus
  • renamed TissueClassifier class to StoppingCriterion
    • all sub classes
    • all related variable names
    • all related file names
  • remove temporary fix, while requirements for cython weren't >=0.21
@pep8speaks

This comment has been minimized.

Copy link

commented Jul 27, 2019

Hello @gabknight, Thank you for updating !

Line 92:1: E402 module level import not at top of file
Line 144:1: E402 module level import not at top of file

Line 20:1: E402 module level import not at top of file
Line 21:1: E402 module level import not at top of file
Line 22:1: E402 module level import not at top of file
Line 23:1: E402 module level import not at top of file
Line 24:1: E402 module level import not at top of file
Line 26:1: E402 module level import not at top of file
Line 27:1: E402 module level import not at top of file
Line 28:1: E402 module level import not at top of file
Line 29:1: E402 module level import not at top of file
Line 30:1: E402 module level import not at top of file
Line 31:1: E402 module level import not at top of file

Line 30:1: E402 module level import not at top of file
Line 31:1: E402 module level import not at top of file
Line 32:1: E402 module level import not at top of file
Line 33:1: E402 module level import not at top of file
Line 36:1: E402 module level import not at top of file
Line 37:1: E402 module level import not at top of file
Line 38:1: E402 module level import not at top of file
Line 39:1: E402 module level import not at top of file

Line 105:1: E402 module level import not at top of file
Line 156:1: E402 module level import not at top of file

Line 37:1: E402 module level import not at top of file
Line 76:1: E402 module level import not at top of file

Line 29:1: E402 module level import not at top of file
Line 31:1: E402 module level import not at top of file

Line 17:1: E402 module level import not at top of file
Line 18:1: E402 module level import not at top of file
Line 21:1: E402 module level import not at top of file
Line 22:1: E402 module level import not at top of file
Line 23:1: E402 module level import not at top of file
Line 24:1: E402 module level import not at top of file
Line 25:1: E402 module level import not at top of file
Line 27:1: E402 module level import not at top of file
Line 30:1: E402 module level import not at top of file

Line 31:1: E402 module level import not at top of file
Line 36:1: E402 module level import not at top of file
Line 37:1: E402 module level import not at top of file
Line 38:1: E402 module level import not at top of file
Line 39:1: E402 module level import not at top of file
Line 41:1: E402 module level import not at top of file
Line 42:1: E402 module level import not at top of file
Line 43:1: E402 module level import not at top of file
Line 44:1: E402 module level import not at top of file
Line 45:1: E402 module level import not at top of file
Line 48:1: E402 module level import not at top of file

Comment last updated at 2019-07-31 11:28:56 UTC
@codecov-io

This comment has been minimized.

Copy link

commented Jul 27, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1926   +/-   ##
=========================================
  Coverage          ?   82.84%           
=========================================
  Files             ?      118           
  Lines             ?    14701           
  Branches          ?     2333           
=========================================
  Hits              ?    12179           
  Misses            ?     1975           
  Partials          ?      547
Impacted Files Coverage Δ
dipy/tracking/local_tracking.py 95.83% <100%> (ø)
dipy/workflows/tracking.py 96.59% <100%> (ø)

@gabknight gabknight added this to the 1.0 milestone Jul 27, 2019

@gabknight

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

I'm not sure what is the standard here for file names.

  • tissue_classifier.py or tissueclassifier.py
  • localtracking.py or local_tracking.py

Which are better? the bold are the current ones.

@gabknight

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

Waiting on #1925 to fix the examples with this RF.

Do we want to change more here? We briefly discussed the TissueClassifier renaming at some point to disentangle it with dipy.segment.tissue.TissueClassifierHMRF, which are not the same kind of object. That could be for instance MaskingStrategy, BinaryMaskingStratery. ActMaskingStrategy. or StoppingCriterion, ThresholdStoppingCriterion, ActStoppingCriterion. What do you think @Garyfallidis @skoudoro @arokem ?

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Thank you for this work @gabknight! Concerning the naming of TissueClassifier, I like the StoppingCriterion, but I need to discuss with @Garyfallidis.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Concerning the filenames, I am not sure we have a standard rule. But since we are following the pep8, this is what it said:

Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability. Python packages should also have short, all-lowercase names, although the use of underscores is discouraged.

So I will prefer local_tracking.py and tissue_classifier.py

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

#1925 is in, you can rebase @gabknight 😄

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

What about something more generic? BinaryStrategy, ThresholdedStrategy, AnatomicallyConstrainedStrategy etc. We could say StoppingStrategy but too many SSs. And name too long?

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Other ideas? Also we could call it TissueClassification rather than Classifier.

@gabknight

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Thanks @skoudoro @Garyfallidis. I'm fine with both StoppingStrategy or StoppingCriterion. Strategy is not descriptive enough in my opinion and TissueClassifier is also long.

@gabknight gabknight force-pushed the gabknight:RF_local_tracking branch from 329ed6a to 85d0ae7 Jul 30, 2019

@gabknight gabknight changed the title [WIP] RF - dipy.tracking.local RF - dipy.tracking.local Jul 30, 2019

@gabknight

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

With the exception of tissue_classifier renaming. This PR is ready. I updated the description with the main changes. This PR ended up being bigger than I expected, but most changes are simple variable/import renaming.

@skoudoro
Copy link
Member

left a comment

Below some quick comments, I need to install and run it but it seems perfect. We just need to take a decision for TissueClassifier

dipy/tracking/tissue_classifier.pxd Outdated Show resolved Hide resolved
doc/api_changes.rst Outdated Show resolved Hide resolved
@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Fine with StoppingCriterion. Go ahead @gabknight!

@gabknight

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

OK. The renaming is done. Quite a few files modify from this change. Let's see if I missed something.

@skoudoro
Copy link
Member

left a comment

All looks good to me. I installed and ran the test on my mac and everything run well. The last step is to generate the documentation and after that, I will be ok to merge this PR.

It would be good if someone else could look at this PR.

npt.assert_equal(state_boolean, TissueTypes.ENDPOINT)
npt.assert_equal(state_float64, TissueTypes.ENDPOINT)
npt.assert_equal(state_boolean, int(StreamlineStatus.ENDPOINT))
npt.assert_equal(state_float64, int(StreamlineStatus.ENDPOINT))

This comment has been minimized.

Copy link
@Garyfallidis

Garyfallidis Jul 31, 2019

Member

@gabknight why this needs casting now?

This comment has been minimized.

Copy link
@gabknight

gabknight Jul 31, 2019

Author Contributor

I don't know why, but on a single build of Travis, with python 3.5, the test was failing because it was comparing a int with a StreamlineStatus Enum type. Somehow, with that build setup, assert_equal wasn't comparing the int value of the Enum but the actual type Enum <StreamlineStatus.ENDPOINT> with the int value on the other side.

Since all other tests passed and all other build worked fine, I just fixed the issue this way. Should I add a comment here?

This comment has been minimized.

Copy link
@gabknight

gabknight Jul 31, 2019

Author Contributor

This was the failing build: Python: 3.5
DEPENDS="cython==0.29 numpy==1.9.0 scipy==1.0 nibabel==2.4.0 h5py==2.4.0"

Maybe the numpy version it too old here to correctly handle this.

This comment has been minimized.

Copy link
@Garyfallidis

Garyfallidis Jul 31, 2019

Member

Makes sense. Thanks

@Garyfallidis Garyfallidis merged commit d1da61a into nipy:master Jul 31, 2019

1 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
frheault added a commit to frheault/dipy that referenced this pull request Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.