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

Fix principal direction extent #1008

Merged
merged 4 commits into from
Mar 21, 2022
Merged

Conversation

eleftherioszisis
Copy link
Contributor

Fixes #1007

@eleftherioszisis
Copy link
Contributor Author

@lidakanari , could you please check that with these changes you get the expected results?

@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2022

Codecov Report

Merging #1008 (c3b3b1d) into master (fdf9f78) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1008   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           36        36           
  Lines         2355      2350    -5     
=========================================
- Hits          2355      2350    -5     

neurom/morphmath.py Outdated Show resolved Hide resolved
Copy link
Contributor

@lidakanari lidakanari left a comment

Choose a reason for hiding this comment

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

The function seems to return correct values for the tested cases. However, the signature of what it returns changes with respect to the previous one, as it now returns a vector of the same dimension as the input data. i.e. 2d point cloud returned 3d vector before, now it returns 2d.

@eleftherioszisis
Copy link
Contributor Author

The function seems to return correct values for the tested cases. However, the signature of what it returns changes with respect to the previous one, as it now returns a vector of the same dimension as the input data. i.e. 2d point cloud returned 3d vector before, now it returns 2d.

Indeed, the dimension of the extents depends on the dimension of the points. Therefore, for 2D points it would return a 2D vector of extents, and for 3D a 3D respectively. For NeuroM purposes, only 3D points are used, so this is not really an issue.

Copy link
Contributor

@lidakanari lidakanari left a comment

Choose a reason for hiding this comment

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

Ok, that should be good. Also something to keep in mind, currently the extents are not sorted (i.e. largest to smallest). If this is by choice, it would be good to mention in documentation because it might be confusing.

@eleftherioszisis
Copy link
Contributor Author

Ok, that should be good. Also something to keep in mind, currently the extents are not sorted (i.e. largest to smallest). If this is by choice, it would be good to mention in documentation because it might be confusing.

That's true indeed. There was no requirement that they are sorted from max to min, which imho renders the direction argument a bit useless in the respective feature.

Should I order them?

@lidakanari
Copy link
Contributor

I think you can implement a "major_extend" feature or order them (but that would be a breaking change).

@eleftherioszisis eleftherioszisis merged commit ba29611 into master Mar 21, 2022
@eleftherioszisis eleftherioszisis deleted the zisis/fix-principal-directions branch March 21, 2022 10:05
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.

Bug in principal direction extent
5 participants