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 seg fault and enable parameter error=0 in FrechetShortcut #1726

Merged
merged 12 commits into from
Jun 10, 2024

Conversation

isivigno
Copy link
Member

PR Description

Fix seg fault due to recent compilers in FrechetShortcut (Bertrand Kerautret, Isabelle Sivignon)
Fix FrechetShortcut to enable the parameter error to be equal to 0 and add new tests in testFrechetShortcut (Isabelle Sivignon)

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug mode.
  • All continuous integration tests pass (Github Actions)

Copy link
Member

@dcoeurjo dcoeurjo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I just have minor comments. best

ChangeLog.md Outdated Show resolved Hide resolved
src/DGtal/geometry/curves/FrechetShortcut.h Show resolved Hide resolved
@dcoeurjo
Copy link
Member

dcoeurjo commented Jun 3, 2024

Hi @isivigno, we are preparing the 1.4 release of DGtal. Could you please have a look to my comments on your PR?
thanks

@dcoeurjo
Copy link
Member

few minor errors in the CI bots. could you please have a look @kerautret ?
(I would like to include this PR in the release)

@kerautret
Copy link
Member

few minor errors in the CI bots. could you please have a look @kerautret ? (I would like to include this PR in the release)

Yes I am adding the PRECISION in the constructor (but also used in static)

@kerautret
Copy link
Member

few minor errors in the CI bots. could you please have a look @kerautret ? (I would like to include this PR in the release)

Yes I am adding the PRECISION in the constructor (but also used in static)

not very nice since it also appears in the cone class, so we should add it in 3 constructors more ..

@kerautret
Copy link
Member

@dcoeurjo for the CI I added a PR on the @isivigno branches isivigno#2

@dcoeurjo
Copy link
Member

I was able to merge he PR on @isivigno repo but the PR was against the master, not the freshet branch. Let me check

@kerautret
Copy link
Member

for the precision constant I can finish the stashed starting inclusion on classes if you think it better

@dcoeurjo
Copy link
Member

let see if this one passes all tests.

@kerautret
Copy link
Member

I was able to merge he PR on @isivigno repo but the PR was against the master, not the freshet branch. Let me check

oupst sure ;)

@dcoeurjo
Copy link
Member

Ok I will merge this one and freeze the repo for the release

@dcoeurjo
Copy link
Member

If you have anything you would like to change on this class, let's do that in the 1.5beta.

thanks

@dcoeurjo dcoeurjo merged commit 2da53eb into DGtal-team:master Jun 10, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants