WIP: Marco/rebase fixes by kayarre · Pull Request #409 · vmtk/vmtk

kayarre · 2022-01-06T21:35:42Z

Rebased branch from @marco-fedele and moved new scripts into the contrib directory.

Changes in other scripts seem minor, but still need to test.

…eference surface exploiting vtkImplicitPolyDataDistance class

…e reference surface)

…al expansion of the bounding box

…t bounding box

…ng them in images

…o in GuessFormat mode

…is active, now only if the array is not defined

… a triangle with the Interpolator

…rking)

… points from the output

…outside region

…o avoid duplicated code

…rection of the new z-axis

… scripts pep8 fixes autopep8 --select W291,W292,W293,W391,E301,E302,E303,E304,E305,E306

lassoan

Thank you. The changes are extensive, but very valuable, as well as your efforts to bring these all back to upstream VMTK.

Have you contacted @marco-fedele? Would he switch to using this repository if these changes are merged? That would be awesome, because then we would not duplicate all the maintenance efforts.

I would not worry too much about what this specific branch potentially breaks, because we have integrated so many other changes that a thorough test campaign (preferably by automated tests) is needed anyway.

kayarre · 2022-01-06T22:17:39Z

I have yet to try reaching out to @marco-fedele. I was hoping he would see this and respond.

marco-fedele · 2022-01-07T17:34:56Z

Dear Kurt and Andras,

For sure I will use the upstream master! Thanks for your interest in including my new scripts!

I will compile this branch and test my scripts as soon as possible.

Do new scripts need to have automatic tests associated with them?

If you are interested, additional information about (most of) new scripts can be found in this paper:

Fedele, M, Quarteroni, A. Polygonal surface processing and mesh generation tools for the numerical simulation of the cardiac function. Int J Numer Meth Biomed Engng. 2021;e3435. https://doi.org/10.1002/cnm.3435

lassoan · 2022-01-07T17:49:36Z

@marco-fedele thanks for showing up here.

Do new scripts need to have automatic tests associated with them?

Yes, that would be great. It would help discovering and fixing regressions and serve as good examples.

If you are interested, additional information about (most of) new scripts can be found in this paper:

Add this as a comment to each of your Python scripts (if you haven't added it already).

move new scripts to contrib directory update cmake and imports chore: pep8 fixes

kayarre · 2022-01-07T21:43:54Z

@marco-fedele if you want to start from this rebased branch that is fine. I just pushed a pep8 fixup commit.

lassoan · 2022-02-28T14:24:16Z

Can you remind what is the status of this? Is this ready to be merged or anybody wanted to further review/discuss it?

marco-fedele · 2022-02-28T15:10:17Z

Can you remind what is the status of this? Is this ready to be merged or anybody wanted to further review/discuss it?

Dear @lassoan ,

sorry, but I was busy with other projects and still didn't have time to focus on finalizing this pull request.

I have just created a new (more updated) pull request #419 to replace this one (so I can edit it).

The PR is still in draft state because I have to finish checking if everything works with the new VTK. I hope to find time to finish the final checks / cleaning in the coming weeks.

Also, instead of rebasing the commits (as @kayarre did in this PR) I have kept everything consistent with my fork history, as there are many contributors / students who have cloned my repository, and if that's not a problem, I'd rather avoid local conflicts.

If it is ok for you, we can close this PR and continue with the new one.

lassoan · 2022-03-01T00:34:54Z

Merge commits introduce lots of noise, making it hard to distinguish upstream changes from actual contributions, and if comments are not written very thoroughly then they obscure the change history. Merge commits may also create merge conflicts that are practically impossible to resolve (for example, we would need to manually redo all the changes in the already very large and complex #273 pull request). Therefore, it would be much better if you could rebase your changes on the latest master version.

However, there are a number of merge commits in the history, so it should be tolerable to add some more. But at least merge commits should have very clear, descriptive commit comment, telling what actually happened in the merged branch (comments such as Merge branch 'master' of github.com:marco-fedele/vmtk should be avoided). If you would want to avoid rebasing on the latest master, would you be at least open to fixing up the top-level merge commit comments?

marco-fedele · 2022-03-01T09:03:36Z

Merge commits introduce lots of noise, making it hard to distinguish upstream changes from actual contributions, and if comments are not written very thoroughly then they obscure the change history. Merge commits may also create merge conflicts that are practically impossible to resolve (for example, we would need to manually redo all the changes in the already very large and complex #273 pull request). Therefore, it would be much better if you could rebase your changes on the latest master version.

However, there are a number of merge commits in the history, so it should be tolerable to add some more. But at least merge commits should have very clear, descriptive commit comment, telling what actually happened in the merged branch (comments such as Merge branch 'master' of github.com:marco-fedele/vmtk should be avoided). If you would want to avoid rebasing on the latest master, would you be at least open to fixing up the top-level merge commit comments?

Ok, don't worry. If this is the policy is not a problem to do the rebase. I have just pushed a rebased version of my changes.

If you agree, we can close this PR and continue in the other one ( #419 ) where I have the permission to contribute.

lassoan · 2022-03-01T14:59:29Z

Thank you very much. I'm closing this pull request then.

marco-fedele added 30 commits January 6, 2022 15:38

useless execution permission in two scripts

6fb4e3c

script to compute signed and unsigned distances on a surface from a r…

786e455

…eference surface exploiting vtkImplicitPolyDataDistance class

little fix to the script

dd1da8d

equivalent implicit distance computation script on a mesh

4268619

new option to have binary output with inside and outside value (of th…

be84bf0

…e reference surface)

little fix: an input parameter was repeated two times

9bceb64

little fix

b913e65

cell data as possible output and always a single array as output

6d1de87

same update in the mesh implicit distance script

394c869

fix in vmtkscripts.py

a1459a1

implicit distance computed from cell center in case of CellData = true

863e5af

creating vmtkmeshrefinement script, but still some problem in execution

fcdd0bf

little fix and comments

ce5fccc

surfacemodeller2 based on vtkImplicitPolyDataDistance and with option…

9f5cd5b

…al expansion of the bounding box

new options: expand only in some directions and correct spacing to fi…

2d55794

…t bounding box

possibility to have a binary image as output

e2cbf7f

error in script name

3bc3f69

correct spacing true as default

6ec7ae6

script to perform boolean operation between surfaces after transformi…

bb6b47d

…ng them in images

lifev extensions format was not correct; vmtkmeshwriter now works als…

b4e16bf

…o in GuessFormat mode

BUG: meshgenerator was resetting cellEntityIdsArray when SkipCapping …

080f91d

…is active, now only if the array is not defined

vtkContourWidget always On, key binding i was not working

b93fedf

SnapToClosestPoint active since it is meaningless having point inside…

fddf76f

… a triangle with the Interpolator

more oprions to vmtksurfaceregiondrawing (celldata array still not wo…

ee9b181

…rking)

vmtksurfaceregiondrawing extended to cell data array

115f6ab

adding cleanoutput option to vmtksurfaceremeshing to clean the unused…

c64082f

… points from the output

vmtksurfaceimplicitdistance: adding the possibility not to overwrite …

d87cc4f

…outside region

vmtkmeshimplicitdistance now exploits the equivalent surface script t…

fb5d951

…o avoid duplicated code

adding an option to vmtkimagereslice to rotate an image fixing the di…

45349af

…rection of the new z-axis

adding options to vmtkimageviewer to activate only some plane widgets

cf2e9e8

marco-fedele and others added 16 commits January 6, 2022 15:38

Changing default capping method

a8d8ddd

Option to extrude along average direction

64f053d

Change dCollapseAngapseAngle in meshcapper script

55032e5

More compact code for WriteHexMshFile

9d2ab2f

vmtkmeshwriter -f dealii/lifex now supports also tetra mesh writing

750c592

A script to delete internal 2D cells from a vtkUnstructuredGrid

a923e08

Fix 'int object not iterable' when a single id is exctracted

7350e1b

Fix elements with negative volume after extrusion

5b9cb6f

Script to merge a set of meshes into a unique one

3d053fd

Fix doc

f145252

Fix compress dolfin mesh writing

0074f24

boolean-connector can skip only final remeshing

bd08d59

Additional NexZDirection option in vmtksurfacetransform

c8ccb4f

Script to add coordinates as point-data arrays on a surface

9422e87

Script to add coordinates as point-data arrays on a mesh

c655b42

STYLE: Remove trailing spaces, add/remove extra blank lines in python…

3551a4e

… scripts pep8 fixes autopep8 --select W291,W292,W293,W391,E301,E302,E303,E304,E305,E306

kayarre force-pushed the marco/rebase_fixes branch from f076722 to c7ab111 Compare January 6, 2022 21:38

lassoan approved these changes Jan 6, 2022

View reviewed changes

fix: move new scripts to contrib directory

ae9a692

move new scripts to contrib directory update cmake and imports chore: pep8 fixes

kayarre force-pushed the marco/rebase_fixes branch from c7ab111 to ae9a692 Compare January 7, 2022 21:42

lassoan closed this Mar 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

WIP: Marco/rebase fixes#409

WIP: Marco/rebase fixes#409
kayarre wants to merge 200 commits into
vmtk:masterfrom
kayarre:marco/rebase_fixes

kayarre commented Jan 6, 2022

Uh oh!

lassoan left a comment

Uh oh!

kayarre commented Jan 6, 2022

Uh oh!

marco-fedele commented Jan 7, 2022 •

edited

Loading

Uh oh!

lassoan commented Jan 7, 2022

Uh oh!

kayarre commented Jan 7, 2022

Uh oh!

lassoan commented Feb 28, 2022

Uh oh!

marco-fedele commented Feb 28, 2022

Uh oh!

lassoan commented Mar 1, 2022

Uh oh!

marco-fedele commented Mar 1, 2022

Uh oh!

lassoan commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

kayarre commented Jan 6, 2022

Uh oh!

lassoan left a comment

Choose a reason for hiding this comment

Uh oh!

kayarre commented Jan 6, 2022

Uh oh!

marco-fedele commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lassoan commented Jan 7, 2022

Uh oh!

kayarre commented Jan 7, 2022

Uh oh!

lassoan commented Feb 28, 2022

Uh oh!

marco-fedele commented Feb 28, 2022

Uh oh!

lassoan commented Mar 1, 2022

Uh oh!

marco-fedele commented Mar 1, 2022

Uh oh!

lassoan commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marco-fedele commented Jan 7, 2022 •

edited

Loading