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

Ajout de metrique dans AT et approche multiresolution. Noemie Tasca, … #344

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

NoemieTasca
Copy link

@NoemieTasca NoemieTasca commented Sep 5, 2018

[stagiaire de Jacques-Olivier Lachaud]

PR not ready yet. Working on it.

Thanks a lot for contributing to DGtalTools, before submitting your PR, please fill up the description and make sure that all checkboxes are checked.

PR Description

Metric : Taking into account the metric in the discrete formulation of Ambrosio-Tortorelli.
Multiresolution : Adding a multi-scale resolution in image restoration using Ambrosio-Tortorelli.

Checklist

  • Doxygen documentation of the code completed (classes, methods, types, members...).
  • Main tool doxygen documentation (following existing documentation of DGtalTools documentation.
  • Check if it follows the tools structure described in CONTRIBUTING.md
  • New entry in the ChangeLog.md added.
  • Update the readme with potentially a screenshot of the tools if it applies.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).

@JacquesOlivierLachaud
Copy link
Member

Hi Noemie ! For the last three points, they are easily handled:

  • ChangeLog.md : you can just edit the file, add your PR number and name and a short sentence describing your work.
  • Readme.md: again, it is just editing the file and add a screenshot for marketing (not sure it is necessary since your tool produces the same output as at-u2-v0
  • No warning in debug mode : you can toggle it on since the CI has passed.
    I'll have a look at your PR next week.
    Thanks

@JacquesOlivierLachaud JacquesOlivierLachaud self-assigned this Sep 16, 2018
@kerautret
Copy link
Member

Hi @NoemieTasca @JacquesOlivierLachaud do you think that it is ready for review ? (or better waiting after 1.0 ?)

@kerautret
Copy link
Member

ping @JacquesOlivierLachaud can I review it ? I suppose yes since the check are greens ;)

@kerautret kerautret self-requested a review May 7, 2020 11:46
@JacquesOlivierLachaud
Copy link
Member

Honnestly I had completely forgotten this PR and even the commits I made.
I think you can have a look. This code is sufficiently independent of other programs to be merged quite safely.

@kerautret kerautret added this to the 1.1 milestone May 7, 2020
Copy link
Member

@kerautret kerautret left a comment

Choose a reason for hiding this comment

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

@JacquesOlivierLachaud I get the same error of #354 since I have set the ITK option in DGtal.

/Users/kerautre/EnCours/DGtal/src/DGtal/math/linalg/CVectorSpace.h:103:11: error: no member named 'clear' in 'itkeigen::Matrix<double, -1, -1, 0, -1, -1>' z.clear();

When I unset it worlds fine, but if you see the issue mentioned by @phcerdan perhaps it is a good occasion to check it else no problem to merge and link it it in issue for later. Just fix the changelog with @NoemieTasca name ;)

imageProcessing/at-u2-v0-m.cpp Outdated Show resolved Hide resolved
imageProcessing/at-u2-v0-m.cpp Outdated Show resolved Hide resolved
@JacquesOlivierLachaud
Copy link
Member

I have to check this PR more. Noemie had a lot of weird problem when doing multi scale approaches. I do not know yet if it is a conceptual problem or simply a bug.

@kerautret
Copy link
Member

@NoemieTasca since news changes with CLI prog options were made, to help I PR on your branch the merge: NoemieTasca#1
You can perhaps merge to simplify the review ? (the conflict was not very complicated) (CC @JacquesOlivierLachaud )

@kerautret
Copy link
Member

In the PR I also apply new change with CL11 in link to previous PR.

@kerautret kerautret modified the milestones: 1.1, 1.4 Dec 2, 2022
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