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 current status (issue #100) #108

Merged
merged 6 commits into from
Oct 4, 2023

Conversation

ThibaultLejemble
Copy link
Collaborator

This PR mainly fixes issue #100
It also improves

  • the doc on FIT_RESULT
  • the primitive base finalize
  • the check on conflict

I have just a few questions or remarks

  • the number of neighbors used to determine if a fit is stable or not should depends on the dimension right? For example, a PCA in 2D is stable with 2 neighbors, in 3D with 3 neighbors, etc... I wanted to be sure if it is correct for every fitting procedure and if it is pertinent to add this
  • note that fitting an oriented sphere to 1 point and 1 normal vector in any dimension is always stable as it simply returns the plane that passes by this point with a normal equal to the normal of the input point. For 2 points in 2D I believe it is also quite stable in general. That being said, I am not sure for 2 points in 3D, the sphere fit may be good but the derivatives, the GLS, etc, may not be as good, and the stability may depends on the weighting function that is used, so it is safe to keep these thresholds in the end
  • the file linePrimitive.h has the Primive kew-work in its title, whereas other primitives do not have it. For consistency, should we add it to the plane and algebraic sphere file name, or should we remove it from the line file name?

@ThibaultLejemble ThibaultLejemble added bug Something isn't working question Further information is requested labels Aug 4, 2023
@ThibaultLejemble ThibaultLejemble self-assigned this Aug 4, 2023
nmellado
nmellado previously approved these changes Aug 4, 2023
@nmellado
Copy link
Contributor

nmellado commented Aug 4, 2023

the number of neighbors used to determine if a fit is stable or not should depends on the dimension right? For example, a PCA in 2D is stable with 2 neighbors, in 3D with 3 neighbors, etc... I wanted to be sure if it is correct for every fitting procedure and if it is pertinent to add this

Right, good point. Dim is a good threshold.

note that fitting an oriented sphere to 1 point and 1 normal vector in any dimension is always stable as it simply returns the plane that passes by this point with a normal equal to the normal of the input point. For 2 points in 2D I believe it is also quite stable in general. That being said, I am not sure for 2 points in 3D, the sphere fit may be good but the derivatives, the GLS, etc, may not be as good, and the stability may depends on the weighting function that is used, so it is safe to keep these thresholds in the end

The derivative class (or any other extension) can have a different instability threshold than the main class.

the file linePrimitive.h has the Primive kew-work in its title, whereas other primitives do not have it. For consistency, should we add it to the plane and algebraic sphere file name, or should we remove it from the line file name?

Yes, right. Another option would be to have Primitive directory, and remove "primitive" in the file name. We could also add a Fit directory, and other files at the root. It would make the code structure more obvious, what do you think ?

@ThibaultLejemble
Copy link
Collaborator Author

I will use Dim for the neighborhood thresholds that are currently used.
Good idea for the folders Fit and Primitive, should I add this in this PR, or in another one?

@ThibaultLejemble
Copy link
Collaborator Author

For all the sphere fit, the status is UNDEFINED with less than 3 neighbors, and UNSTABLE with less than 6.
Do you know why the 6 threshold?
For now I replaced the 3 by Dim, and the 6 by 2*Dim

@nmellado
Copy link
Contributor

nmellado commented Oct 3, 2023

@ThibaultLejemble could you please fix the naming issue that makes the tests failing (with Dim) ?

Good idea for the folders Fit and Primitive, should I add this in this PR, or in another one ?

Can be in another one.

Copy link
Contributor

@nmellado nmellado left a comment

Choose a reason for hiding this comment

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

Seems to be ready for merging (after Dim fix).
Please rebase on top of master

seperate the case where the Base class is not STABLE (then the current status is simply returned) and where there is not enough neighbors (then the current status is set to UNDEFINED and returned)
- remove comments about the mean

- remove call to init() since it does nothing in this case
remove neighbor count threshold that depends on fitting procedures
@ThibaultLejemble ThibaultLejemble marked this pull request as ready for review October 4, 2023 09:14
@nmellado nmellado merged commit 75c4b16 into poncateam:master Oct 4, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants