WIP: Refactoring some of the functions in roi.py #302
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks like this is the first time we are using pandas here |
| return pd.DataFrame(series_dict) | ||
|
|
||
|
|
||
| def _mean_intensity(images, labeled_array, index=None): |
There was a problem hiding this comment.
@ericdill I saw you changed function name, you have to change that in test too, because I put a test for that function
|
@ericdill Other than fixing the tests this function looks fine to me, when you fix it I will check it again |
| @@ -15,7 +15,8 @@ before_install: | |||
| install: | |||
| - export GIT_FULL_HASH=`git rev-parse HEAD` | |||
| - conda update conda --yes | |||
| - conda create -n testenv --yes pip nose python=$TRAVIS_PYTHON_VERSION lmfit xraylib numpy scipy scikit-image netcdf4 six coverage | |||
| - conda create -n testenv --yes pip nose python=$TRAVIS_PYTHON_VERSION lmfit xraylib numpy scipy scikit-image netcdf4 six coverage pandas | |||
| >>>>>>> MNT: Add pandas as a dep | |||
Uncovered Suggestions
|
|
@sameera2004 can you review my changes? |
|
ignore failing travis for now. I want to get feedback from chx about this API before we sort out the best way to test these functions |
| @@ -87,3 +87,6 @@ doc/resource/api/generated/ | |||
| *.txt | |||
| *.zip | |||
| *.jpg | |||
|
|
|||
| # ctags | |||
| .tags* | |||
There was a problem hiding this comment.
What is your question here?
it now takes a list of bad data that should be removed before data is returned from the function as a list of rois
It now returns a dataframe
Using pandas drop row/col functions instead
Used to return np.matrix, now returns np.vstack
We should test the public function that uses the private function, not the private function itself.
@danielballan made a convincing argument that the library functions of skxray should not return DataFrames. If we want/need DataFrames, then fancy callable classes could be used to do this, but those callable classes still need to call library functions to do all their work
|
superseded by #306 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Companion PR to Nikea/xray-vision#68