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

Geometry_Engine: Singular Value Decomposition implemented and applied to FitLine #3280

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

pawelbaran
Copy link
Member

@pawelbaran pawelbaran commented Feb 12, 2024

Issues addressed by this PR

Closes #3278

Test files

General tests are available here
Edge cases covered here

Changelog

Additional comments

Issue originally raised by @peterjamesnugent, but he is still on leave, so adding @FraserGreenroyd and @albinber - please let it hang and leave it to the originator if not enough head space, no rush on this one 👍

EDIT: running into more and more issues with line fitting, I started gathering edge cases. I spent lots of effort trying to get them all covered using the existing analytical solution, but failed, which led me to rewrite FitLine method entirely by switching to numerical solution and adding an implementation of Singular Value Decomposition, inspired by this and that. Now the method is much more robust, which can be seen in the test files.

A follow-up feature would be #3287.

tolerance input to be removed from FitLine in a separate PR, see #3288.

@pawelbaran pawelbaran added the type:bug Error or unexpected behaviour label Feb 12, 2024
@pawelbaran pawelbaran self-assigned this Feb 12, 2024
@pawelbaran
Copy link
Member Author

@BHoMBot check required

Copy link

bhombot-ci bot commented Feb 12, 2024

@pawelbaran to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

@pawelbaran pawelbaran changed the title Geometry_Engine: Edge case for FitLine with three equidistant points fixed Geometry_Engine: Singular Value Decomposition implemented and applied to FitLine Feb 17, 2024
@pawelbaran
Copy link
Member Author

@BHoMBot check compliance

Copy link

bhombot-ci bot commented Feb 17, 2024

@pawelbaran to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

Copy link

bhombot-ci bot commented Feb 17, 2024

FAO: @FraserGreenroyd
@pawelbaran is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is code-compliance.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 21690504985

1 similar comment
Copy link

bhombot-ci bot commented Feb 17, 2024

FAO: @FraserGreenroyd
@pawelbaran is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is code-compliance.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 21690504985

@pawelbaran
Copy link
Member Author

@BHoMBot check compliance

Copy link

bhombot-ci bot commented Feb 17, 2024

@pawelbaran to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

Copy link

bhombot-ci bot commented Feb 17, 2024

FAO: @FraserGreenroyd
@pawelbaran is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is documentation-compliance.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 21690575707

@pawelbaran
Copy link
Member Author

@FraserGreenroyd I requested dispensation against document compliance because it seems to be a bug on the side of the bot.

@pawelbaran
Copy link
Member Author

@BHoMBot check required

Copy link

bhombot-ci bot commented Feb 18, 2024

@pawelbaran to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

There are 9 requests in the queue ahead of you.

Copy link

bhombot-ci bot commented Feb 18, 2024

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

I have run through the test scripts provided and scanned through the code and despite being overly maths heavy, not a lot seems out of place so happy to merge this to unblock #3283 and other work for this sprint.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 21690575707

Copy link

bhombot-ci bot commented Feb 19, 2024

@FraserGreenroyd I have now provided a passing check on reference 21690575707 as requested.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented Feb 19, 2024

@FraserGreenroyd to confirm, the following actions are now queued:

  • check ready-to-merge

@FraserGreenroyd
Copy link
Contributor

@BHoMBot this is a DevOps instruction. I am requesting neutral checks on: unit-tests

Copy link

bhombot-ci bot commented Feb 19, 2024

@FraserGreenroyd I have provided neutral checks to the checks requested. These checks will need to be run properly to obtain full results.

Copy link

bhombot-ci bot commented Feb 19, 2024

FAO: @FraserGreenroyd
@FraserGreenroyd is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is documentation-compliance.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 21691286837

@FraserGreenroyd
Copy link
Contributor

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 21691286837

Copy link

bhombot-ci bot commented Feb 19, 2024

@FraserGreenroyd I have now provided a passing check on reference 21691286837 as requested.

@FraserGreenroyd FraserGreenroyd merged commit 9cc40cd into develop Feb 19, 2024
13 checks passed
@FraserGreenroyd FraserGreenroyd deleted the BHoM_Engine-#3278-FitLineEdgeCase branch February 19, 2024 11:44
@bhombot-ci bhombot-ci bot mentioned this pull request Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geometry_Engine: Edge case for FitLine with three equidistant points
2 participants