From: Derek G. <fri...@gm...> - 2009-04-21 20:11:49
|
On Tue, Apr 21, 2009 at 1:40 PM, Derek Gaston <fri...@gm...> wrote: > I'm up for any suggestions about what to do. Maybe we should allow you to > pass in a subdomain_id and starting sideset number to build_cube() or the > like? > Ok - I've implemented this by changing build_cube like the following: void build_cube (UnstructuredMesh& mesh, const unsigned int nx=0, const unsigned int ny=0, const unsigned int nz=0, const Real xmin=0., const Real xmax=1., const Real ymin=0., const Real ymax=1., const Real zmin=0., const Real zmax=1., const ElemType type=INVALID_ELEM, const bool gauss_lobatto_grid=false, const subdomain_id_type subdomain_id=0, const short int initial_sideset_id=0); Adding subdomain_id and initial_sideset_id. I can verify that this works... and allows Exodus output of the generated mesh. Anyone have objections to me checking this in? Derek |
From: Roy S. <roy...@ic...> - 2009-04-21 20:21:43
|
On Tue, 21 Apr 2009, Derek Gaston wrote: > On Tue, Apr 21, 2009 at 1:40 PM, Derek Gaston <fri...@gm...> wrote: > >> I'm up for any suggestions about what to do. Maybe we should allow you to >> pass in a subdomain_id and starting sideset number to build_cube() or the >> like? > > Ok - I've implemented this by changing build_cube like the following: > > void build_cube (UnstructuredMesh& mesh, > const unsigned int nx=0, > const unsigned int ny=0, > const unsigned int nz=0, > const Real xmin=0., const Real xmax=1., > const Real ymin=0., const Real ymax=1., > const Real zmin=0., const Real zmax=1., > const ElemType type=INVALID_ELEM, > const bool gauss_lobatto_grid=false, > const subdomain_id_type subdomain_id=0, > const short int initial_sideset_id=0); > > Adding subdomain_id and initial_sideset_id. I can verify that this works... > and allows Exodus output of the generated mesh. > > Anyone have objections to me checking this in? Seems kind of special-case, doesn't it? I'd much prefer to call build_cube, then set_all_subdomain_ids, then set_all_side_ids (or whatever you want to name the latter two functions; a "set_*_id" that took elem iterators would be better than "set_all_*"). I don't like the idea of two additional arguments that we have to add to every function that generates a mesh. --- Roy |
From: John P. <jwp...@gm...> - 2009-04-21 20:53:18
|
On Tue, Apr 21, 2009 at 3:21 PM, Roy Stogner <roy...@ic...> wrote: > I don't like > the idea of two additional arguments that we have to add to every > function that generates a mesh. Well... neither do I but he's using default arguments, right? It will probably be more work but a generic set_all_subdomain_ids() type of function would be preferable in my opinion as well. -- John |
From: Roy S. <roy...@ic...> - 2009-04-21 21:15:35
|
On Tue, 21 Apr 2009, John Peterson wrote: > On Tue, Apr 21, 2009 at 3:21 PM, Roy Stogner <roy...@ic...> wrote: >> I don't like >> the idea of two additional arguments that we have to add to every >> function that generates a mesh. > > Well... neither do I but he's using default arguments, right? It's not API backwards compatibility I'm worried about - Derek's patch did look backwards compatible. It's API simplicity. Derek added those default_arguments to build_cube. And presumably in build_line/build_square. I'll bet not build_delaunay_square. Either way, do we add the same default arguments to build_sphere? TriangleInterface::triangulate()? Mesh::read()? We've got lots of places where we generate meshes that will default to subdomain ids 0, boundary ids 0. So we need a convenient way to change those ids. A separate function is the most flexible way. And if we have a separate function with a clean enough API, there's no reason to change all the stuff that might precede it from functionA(argsA) to functionA(argsA, then_do_function_B=true). Also: this was all prompted by the discovery that Exodus doesn't allow 0 as a valid id? What if we pick the largest valid id they do support and map 0 to/from that? It's kind of a hack, but then most users would be able to use Exodus IO on arbitrary meshes without changing their code at all. --- Roy |
From: Derek G. <fri...@gm...> - 2009-04-21 21:38:07
|
On Apr 21, 2009, at 3:15 PM, Roy Stogner wrote: > It's not API backwards compatibility I'm worried about - Derek's patch > did look backwards compatible. It's API simplicity. > > Derek added those default_arguments to build_cube. And presumably in > build_line/build_square. I'll bet not build_delaunay_square. Either > way, do we add the same default arguments to build_sphere? > TriangleInterface::triangulate()? Mesh::read()? I actually haven't added it to any others yet... as I was waiting for input. > We've got lots of places where we generate meshes that will default to > subdomain ids 0, boundary ids 0. So we need a convenient way to > change those ids. A separate function is the most flexible way. And > if we have a separate function with a clean enough API, there's no > reason to change all the stuff that might precede it from > functionA(argsA) to functionA(argsA, then_do_function_B=true). The trouble is that it's somewhat difficult to change the boundary ids after they have been set. Your suggestion was my original thought as well... but once you look at boundary_info you realize it's kind of non-trivial to wholesale bump all of the boundary ids up by one... they are kept track of in a couple of different places. Note that I don't say impossible... but you know how lazy I am. ;-) It just seemed easier to modify the root of the problem.... > Also: this was all prompted by the discovery that Exodus doesn't allow > 0 as a valid id? What if we pick the largest valid id they do support > and map 0 to/from that? It's kind of a hack, but then most users > would be able to use Exodus IO on arbitrary meshes without changing > their code at all. Now this is something I completely agree with. The reason I didn't go down this path is because it would cause breakages in everyone's code that uses build_cube and boundary_ids currently... and it would do so silently (ie you would just quietly be enforcing BC's on the wrong sides of your domain). If this is acceptable.... and we can just make it clear this this change is coming to our user community... I'm all for it. Let me know, Derek |
From: Roy S. <roy...@ic...> - 2009-04-21 21:56:00
|
On Tue, 21 Apr 2009, Derek Gaston wrote: > On Apr 21, 2009, at 3:15 PM, Roy Stogner wrote: > >> We've got lots of places where we generate meshes that will default to >> subdomain ids 0, boundary ids 0. So we need a convenient way to >> change those ids. A separate function is the most flexible way. And >> if we have a separate function with a clean enough API, there's no >> reason to change all the stuff that might precede it from >> functionA(argsA) to functionA(argsA, then_do_function_B=true). > > The trouble is that it's somewhat difficult to change the boundary ids after > they have been set. Which is something we ought to fix, but since by "we" I mean "someone other than me", I won't push for this as a solution to your immediate problem. > Your suggestion was my original thought as well... but > once you look at boundary_info you realize it's kind of non-trivial to > wholesale bump all of the boundary ids up by one... they are kept track of in > a couple of different places. Loop from elem iterator first to last; on each element, call BoundaryInfo::remove(elem) and then loop over all boundary sides and call BoundaryInfo::add_side() for each side? I must be missing something - this seems easier than adding new build_* arguments would have been. >> Also: this was all prompted by the discovery that Exodus doesn't allow >> 0 as a valid id? What if we pick the largest valid id they do support >> and map 0 to/from that? It's kind of a hack, but then most users >> would be able to use Exodus IO on arbitrary meshes without changing >> their code at all. > > Now this is something I completely agree with. The reason I didn't go down > this path is because it would cause breakages in everyone's code that uses > build_cube and boundary_ids currently... and it would do so silently (ie you > would just quietly be enforcing BC's on the wrong sides of your domain). It shouldn't break any existing code. If they write out meshes with bcid=0, existing code is already broken, and getting *some* unique id for the bcid=0 boundaries is better than failing to write the file. If they read in meshes, the fact that bcid=0 isn't supported means we know they're not reading in bcid=0. If they read in bcid=(uint)(-1) meshes... well, that will break silently. But I'll bet it never really happens: who's using 2^whatever-1 as a meaningful bcid? > If this is acceptable.... and we can just make it clear this this change is > coming to our user community... I'm all for it. I'd like to hear from Ben, but while this doesn't sound like an ideal solution to me (ideal would be if Exodus natively supported bcid=0 somehow) it sounds optimal under the circumstances. --- Roy |
From: Derek G. <fri...@gm...> - 2009-04-22 18:34:49
|
(Note... restricted to Devel list) On Apr 21, 2009, at 3:55 PM, Roy Stogner wrote: >> Your suggestion was my original thought as well... but once you >> look at boundary_info you realize it's kind of non-trivial to >> wholesale bump all of the boundary ids up by one... they are kept >> track of in a couple of different places. > > Loop from elem iterator first to last; on each element, call > BoundaryInfo::remove(elem) and then loop over all boundary sides and > call BoundaryInfo::add_side() for each side? I must be missing > something - this seems easier than adding new build_* arguments would > have been. Loop over what boundary sides? If you call remove(Elem) it removes all information about the sides from that element. If you mean loop over every element and look for sides with null neighbors... we could do that... but now you don't know what boundary number that side was supposed to have. Probably the easiest thing would be to add a method to BoundaryInfo... like increment_boundary_ids()... that would run through the multimaps and increment the boundary ids. The reason I didn't go this route is that it felt REALLY specialized. I could at least see someone who wasn't even using Exodus using the changes I made to build_cube... but it's hard to imagine anyone else calling this BoundaryInfo method. >>> Also: this was all prompted by the discovery that Exodus doesn't >>> allow >>> 0 as a valid id? What if we pick the largest valid id they do >>> support >>> and map 0 to/from that? It's kind of a hack, but then most users >>> would be able to use Exodus IO on arbitrary meshes without changing >>> their code at all. >> >> Now this is something I completely agree with. The reason I didn't >> go down this path is because it would cause breakages in everyone's >> code that uses build_cube and boundary_ids currently... and it >> would do so silently (ie you would just quietly be enforcing BC's >> on the wrong sides of your domain). > > It shouldn't break any existing code. If they write out meshes with > bcid=0, existing code is already broken, and getting *some* unique id > for the bcid=0 boundaries is better than failing to write the file. > If they read in meshes, the fact that bcid=0 isn't supported means we > know they're not reading in bcid=0. If they read in bcid=(uint)(-1) > meshes... well, that will break silently. But I'll bet it never > really happens: who's using 2^whatever-1 as a meaningful bcid? Oh - sorry I misunderstood what you were getting at. I thought you were saying that we just replace all of the mesh generation code all throughout the library to start numbering with 1 instead of zero... which would obviously break a lot of user code. I don't like this idea at all. We frequently read the solution (and subdomains AND bc_ids) out of Exodus files to start other calculations. This suggestion would have us running the original computation using subdomains of 0 and bc_ids of zero... and then we would have to know to change the input file for the next computation so that materials were matched with subdomain 65536 (or whatever) and BC's with boundary_ids of 65536. That's not much fun at all. > I'd like to hear from Ben, but while this doesn't sound like an ideal > solution to me (ideal would be if Exodus natively supported bcid=0 > somehow) it sounds optimal under the circumstances. I would like to hear from Ben as well. Unfortunately... I need this capability... like yesterday. My local code is already depending on this working... so I can't commit it to our local repositories.... Not a big deal for a little while.... but let's figure this out. Derek |
From: John P. <jwp...@gm...> - 2009-04-22 19:07:37
|
On Wed, Apr 22, 2009 at 1:33 PM, Derek Gaston <fri...@gm...> wrote: > (Note... restricted to Devel list) > > On Apr 21, 2009, at 3:55 PM, Roy Stogner wrote: > >>> Your suggestion was my original thought as well... but once you >>> look at boundary_info you realize it's kind of non-trivial to >>> wholesale bump all of the boundary ids up by one... they are kept >>> track of in a couple of different places. >> >> Loop from elem iterator first to last; on each element, call >> BoundaryInfo::remove(elem) and then loop over all boundary sides and >> call BoundaryInfo::add_side() for each side? I must be missing >> something - this seems easier than adding new build_* arguments would >> have been. > > Loop over what boundary sides? If you call remove(Elem) it removes > all information about the sides from that element. If you mean loop > over every element and look for sides with null neighbors... we could > do that... but now you don't know what boundary number that side was > supposed to have. You could save all the original side ID's for the element before calling BoundaryInfo::remove(elem), and then add them all back with the "right" ID, which may be application-dependent (maybe the user could provide an int array for converting original to new IDs?). For example, you could bump side ID's 0-3 up by one using the array [1 2 3 4]... -- John |
From: Roy S. <roy...@ic...> - 2009-04-22 19:33:59
|
On Wed, 22 Apr 2009, Derek Gaston wrote: > On Apr 21, 2009, at 3:55 PM, Roy Stogner wrote: >> >> Loop from elem iterator first to last; on each element, call >> BoundaryInfo::remove(elem) and then loop over all boundary sides and >> call BoundaryInfo::add_side() for each side? I must be missing >> something - this seems easier than adding new build_* arguments would >> have been. > > Loop over what boundary sides? If you call remove(Elem) it removes all > information about the sides from that element. Right. The "set_*" APIs I was talking about would be to set ids to a user-specified number if the elements matched a certain condition; you wouldn't need the old boundary ids for that. The context was setting every id to 1, remember - at that point we don't care to make sure that the id was 0 before. > If you mean loop over every element and look for sides with null > neighbors... we could do that... but now you don't know what > boundary number that side was supposed to have. If you want an "increment_*" or "set_bcid_from_x_to_y" type method instead, that's not too hard either. In fact, see ~roystgnr/trunk/meshbcid/ for code that does exactly the latter. > Probably the easiest thing would be to add a method to BoundaryInfo... like > increment_boundary_ids()... that would run through the multimaps and > increment the boundary ids. The reason I didn't go this route is that it > felt REALLY specialized. I'd have gone with "set_from_x_to_y" (with some better name: "remap_id"?) instead, but there's nothing wrong with using specialized code if we've got a specialized problem. Exodus IO needs to do something with 0 ids; BoundaryInfo might be the best place to implement that something. For increment_boundary_ids() I'd probably agree that it's not worth adding to BoundaryInfo; just do it in a few lines in the Exodus code. But remap_id() is useful enough to be worth exposing to other users. > I could at least see someone who wasn't even using > Exodus using the changes I made to build_cube... but it's hard to imagine > anyone else calling this BoundaryInfo method. increment_boundary_ids(), no. remap_id(), definitely - I've had to use it before (hence the "meshbcid" utility) and IIRC others have too. >> It shouldn't break any existing code. If they write out meshes with >> bcid=0, existing code is already broken, and getting *some* unique id >> for the bcid=0 boundaries is better than failing to write the file. >> If they read in meshes, the fact that bcid=0 isn't supported means we >> know they're not reading in bcid=0. If they read in bcid=(uint)(-1) >> meshes... well, that will break silently. But I'll bet it never >> really happens: who's using 2^whatever-1 as a meaningful bcid? > > Oh - sorry I misunderstood what you were getting at. I thought you were > saying that we just replace all of the mesh generation code all throughout > the library to start numbering with 1 instead of zero... which would > obviously break a lot of user code. Right. That would be way too disruptive. > I don't like this idea at all. We frequently read the solution (and > subdomains AND bc_ids) out of Exodus files to start other calculations. This > suggestion would have us running the original computation using subdomains of > 0 and bc_ids of zero... No; this suggestion is *in addition* to the remap_id() suggestion. Use remap_id to fix your problem; use a combination of remap_id and fixed Exodus IO to prevent others from experiencing the same problem. The fix means that if they want to write an arbitrary mesh to an Exodus file it won't break; the remap_id functionality means that if they want to create a mesh or write an Exodus file with specific ids they can do so more easily. > and then we would have to know to change the input file for the next > computation so that materials were matched with subdomain 65536 (or > whatever) and BC's with boundary_ids of 65536. That's not much fun > at all. Right. In your case you'd remap_id(0, whatever_you_wanted). The default value for 0 would be for maximum compatibility with people who are using libMesh ids as they are but who don't want an Exodus write to crash on them. --- Roy |
From: Derek G. <fri...@gm...> - 2009-04-22 19:40:50
|
Understood... I'll make it happen. Derek On Apr 22, 2009, at 1:33 PM, Roy Stogner wrote: > > On Wed, 22 Apr 2009, Derek Gaston wrote: > >> On Apr 21, 2009, at 3:55 PM, Roy Stogner wrote: >>> Loop from elem iterator first to last; on each element, call >>> BoundaryInfo::remove(elem) and then loop over all boundary sides and >>> call BoundaryInfo::add_side() for each side? I must be missing >>> something - this seems easier than adding new build_* arguments >>> would >>> have been. >> >> Loop over what boundary sides? If you call remove(Elem) it removes >> all information about the sides from that element. > > Right. The "set_*" APIs I was talking about would be to set ids to a > user-specified number if the elements matched a certain condition; you > wouldn't need the old boundary ids for that. The context was setting > every id to 1, remember - at that point we don't care to make sure > that the id was 0 before. > >> If you mean loop over every element and look for sides with null >> neighbors... we could do that... but now you don't know what >> boundary number that side was supposed to have. > > If you want an "increment_*" or "set_bcid_from_x_to_y" type method > instead, that's not too hard either. In fact, see > ~roystgnr/trunk/meshbcid/ for code that does exactly the latter. > >> Probably the easiest thing would be to add a method to >> BoundaryInfo... like increment_boundary_ids()... that would run >> through the multimaps and increment the boundary ids. The reason I >> didn't go this route is that it felt REALLY specialized. > > I'd have gone with "set_from_x_to_y" (with some better name: > "remap_id"?) instead, but there's nothing wrong with using specialized > code if we've got a specialized problem. Exodus IO needs to do > something with 0 ids; BoundaryInfo might be the best place to > implement that something. For increment_boundary_ids() I'd probably > agree that it's not worth adding to BoundaryInfo; just do it in a few > lines in the Exodus code. But remap_id() is useful enough to be worth > exposing to other users. > >> I could at least see someone who wasn't even using Exodus using the >> changes I made to build_cube... but it's hard to imagine anyone >> else calling this BoundaryInfo method. > > increment_boundary_ids(), no. remap_id(), definitely - I've had to > use it before (hence the "meshbcid" utility) and IIRC others have too. > >>> It shouldn't break any existing code. If they write out meshes with >>> bcid=0, existing code is already broken, and getting *some* unique >>> id >>> for the bcid=0 boundaries is better than failing to write the file. >>> If they read in meshes, the fact that bcid=0 isn't supported means >>> we >>> know they're not reading in bcid=0. If they read in bcid=(uint)(-1) >>> meshes... well, that will break silently. But I'll bet it never >>> really happens: who's using 2^whatever-1 as a meaningful bcid? >> >> Oh - sorry I misunderstood what you were getting at. I thought you >> were saying that we just replace all of the mesh generation code >> all throughout the library to start numbering with 1 instead of >> zero... which would obviously break a lot of user code. > > Right. That would be way too disruptive. > >> I don't like this idea at all. We frequently read the solution >> (and subdomains AND bc_ids) out of Exodus files to start other >> calculations. This suggestion would have us running the original >> computation using subdomains of 0 and bc_ids of zero... > > No; this suggestion is *in addition* to the remap_id() suggestion. > Use > remap_id to fix your problem; use a combination of remap_id and fixed > Exodus IO to prevent others from experiencing the same problem. The > fix means that if they want to write an arbitrary mesh to an Exodus > file it won't break; the remap_id functionality means that if they > want to create a mesh or write an Exodus file with specific ids they > can do so more easily. > >> and then we would have to know to change the input file for the next >> computation so that materials were matched with subdomain 65536 (or >> whatever) and BC's with boundary_ids of 65536. That's not much fun >> at all. > > Right. In your case you'd remap_id(0, whatever_you_wanted). The > default value for 0 would be for maximum compatibility with people who > are using libMesh ids as they are but who don't want an Exodus write > to crash on them. > --- > Roy |