bugpyFormex - Bugs: bug #40662, selectProp and related have no...

 
 

bug #40662: selectProp and related have no option for compacting

Submitter:  francesco <francio>
Submitted:  Thu 21 Nov 2013 05:11:40 PM UTC
   
 
Category:  Core Severity:  3 - Normal
Item Group:  Feature request Status:  Fixed
Privacy:  Public Assigned to:  bverheg
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 22 May 2014 07:32:54 PM UTC, comment #10: 

In 8576bb9 the default for (c)selectProp has also been changed to False. This closes the bug.

Benedict Verhegghe <bverheg>
Group administrator
Thu 22 May 2014 03:47:02 PM UTC, comment #9: 

In 44dec27, compact=False has been made the default for select and cselect. We probably should do this also for selectProp and cselectProp.

Benedict Verhegghe <bverheg>
Group administrator
Tue 13 May 2014 06:38:25 AM UTC, comment #8: 

Hi
This bug can be closed. Though the suggestion of benedict have been implemented
select/cselect still use compact=True

Benedict can you close this bug and implement the changes if any?

francesco <francio>
Group Member
Fri 29 Nov 2013 01:19:32 PM UTC, comment #7: 

Since I have not seen a deciding number of responses yet, here is a proposal of what I think to do:

- select/cselect use compact=False by default: they are intended to select some components of a Mesh but the user wants to keep working within the framework of that Mesh.

- clip/cclip become wrapper functions that call select with the option compact=True: they are intended for the user who wants the result of the selection for the continuation of his work.

Other element selection mechanisms should by preference just return a list of element numbers: these can then be put in any of the methods above to yield the corresponding Mesh. All the other methods then do not need a compact= option, nor do they need a complementary operation. And there is the added benefit that you can just get the element number list if that is all you need.

The new adjacentTo method works that way. I think we should change connectedTo in the same sense, and remove notConnectedTo.



Benedict Verhegghe <bverheg>
Group administrator
Mon 25 Nov 2013 09:22:11 AM UTC, comment #6: 

Well, I see two reasons to keep the compact option, even if we make the default False.

- A lot of people now have probably code with 'compact=False' in it. That code would break if we just remove the option.

- In GUI functions, it is now easy to add the option to the input dialog as an _I('compact',False) item and execute the function(**res) with the dialog results. If the option is removed, we have to add everywhere a piece of code


M = function(**res)
if res['compact']:
    M = M.compact()


It should even be more complex: the 'compact' should be removed from the res first, and the value remembered.

Benedict Verhegghe <bverheg>
Group administrator
Mon 25 Nov 2013 09:05:53 AM UTC, comment #5: 

Hi everybody,
I would like to propose the following:

-IMMEDIATELY REMOVE the option compact from (c)select and (c)selectProp, getBorderMesh, getFreeEntitiesMesh,getFreeEdgesMesh,connectedTo. You can perfectly do it afterwards with a .compact() or a .fuse().compact() or better as a .fuse().compact().renumber(), without adding mem or cpu time. Also, other codes such as Abaqus and VTK do not alter the coords when working on connectivity/selection. If you want to compact (and/or renumber) you need to use a 'clean' in VTK and 'removeUnneededNodes' in Abaqus CAE. In a .inp file or .vtk you normally have unused nodes if you do not remove them first. The beginning user will probably not care if his model has unused nodes, but he would probably like that a node nr 3 will always be node nr 3 also after a .getBorderMesh or a .select.

-in functions where the .compact() is not executed at the end, I would keep the compact option. Most of the times it would reduce the memory usage (especially when there are for loops as in splitRandom).

-clipping TriSurface and GUI functions can use a compact() when needed (e.g. after clipping). As already suggested, clip and cclip are redundant (already for a long time actually) and can be replaced by (c)select.



gianluca de santis <gianlucadesi>
Group Member
Mon 25 Nov 2013 08:02:14 AM UTC, comment #4: 

I agree that a default compact=False would give surprising results to the beginning user, especially when using the GUI menu functions (e.g. clipping a surface).

Sofie Van Cauter <svcauter>
Group Member
Sun 24 Nov 2013 04:49:47 PM UTC, comment #3: 

On one hand I think it is better to compact by default. The beginning user would be surprised that a Mesh contains unneeded nodes. We would have to explain the implementation in more detail before he can start using Mesh. Experienced users like yourself will know why, when and where they have to add the compact=False option.

On the other hand, compact=False may be used more than compact=True, so it would make sense to make False the default.
I think however that the default should be the same, not only for selectProp and select, but probably for all the methods having the compact argument. There are currently 14 of them: 13 with True as default, only 1 with False as default.

An asset of setting default=False would be that we can remove the option altogether: it is not more difficult to write M.select(something).compact() than M.select(something,compact=True).

The change to default compact=False (or removal of the compact option) would of course require us to check carefully the usage of the said 14 methods. Some will be no problem: selectProp and cselectProp could be directly set to False, since the withProp also worked that way (is that correct?). clip/cclip can probably be deprecated anyway: they seem to do the same as select/cselect. TriSurface.removeDegenerate can loose the option: Mesh.removeDegenerate has it neither.

So, my idea is the following: if enough developers/users (let's say 5) ask for a default compact=False, I will start changing (and later removing the option).

Otherwise, I will make my one decision, someday.

Benedict Verhegghe <bverheg>
Group administrator
Sun 24 Nov 2013 02:07:02 PM UTC, comment #2: 

It works with the new changes and after commit e978091.
Though I ll suggets to set the deafult compact to False.
Normally while working with properties I  think the user will rather keep the original positions than the compacted mesh.
I left the discussion open only for this matter, else can be closed

francesco <francio>
Group Member
Thu 21 Nov 2013 06:18:01 PM UTC, comment #1: 

In commit 4f238bc a 'compact' argument has been added to selectProp and cselectProp.

Geometry.select has compact=True as default because that is what a beginning user will probably expect. An experienced user will know what this argument is about, and he can set it to False.

If the default True is annoying, we can discuss if it should be changed, or if we need to provide a configuration for the default value.

Notice that select/cselect have been moved from Formex/Mesh up to Geometry.

Please test the correct working of (c)select(Prop) and report.

Benedict Verhegghe <bverheg>
Group administrator
Thu 21 Nov 2013 05:11:40 PM UTC, original submission:  

why Geometry.select  have default compact=True?
moreover selectProp and related have no option for compacting.

If there is no good reason i will restore all defaults to compact=False as it was before with deprecated withProp and related selection tools

francesco <francio>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gianlucadesi (Posted a comment)
  • -email is unavailable- added by svcauter (Posted a comment)
  • -email is unavailable- added by bverheg (Posted a comment)
  • -email is unavailable- added by francio (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-05-22 bverheg Open/ClosedOpen Closed
    2013-11-24 francio StatusReady For Test Fixed
    2013-11-21 bverheg StatusNone Ready For Test
        Assigned toNone bverheg

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code