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 bug in RenameBlockGenerator #17710

Closed
yjung-anl opened this issue Apr 29, 2021 · 9 comments · Fixed by #19386
Closed

Fix bug in RenameBlockGenerator #17710

yjung-anl opened this issue Apr 29, 2021 · 9 comments · Fixed by #19386
Labels
C: Framework P: normal A defect affecting operation with a low possibility of significantly affects. T: defect An anomaly, which is anything that deviates from expectations.

Comments

@yjung-anl
Copy link
Contributor

yjung-anl commented Apr 29, 2021

Bug Description

Found that the block id is not properly re-assigned for certain cases. The following is the case having this issue. The resulting block_id of "RenameBlockGenerator" is "1 2 1" not "3 2 1" as shown in below figures.

[Mesh]
  [a]
    type = ConcentricCircleMeshGenerator
    num_sectors = 4
    radii = '0.4 0.5'
    rings = '2 1 1'
    has_outer_square = on
    pitch = 1.2
    preserve_volumes = yes
    smoothing_max_it = 3
  []

  [b]
    type = RenameBlockGenerator
    input = 'a'
    old_block_id = '1 2 3'
    new_block_id = '3 2 1'
  []
[]

Before applying RenameBlockGenerator (Block: 1 2 3 from center to outer)

After applying RenameBlockGenerator (Block: 1 2 1 from center to outer)

Steps to Reproduce

It can be reproduced for the input case attached above.

Impact

Fix bug in RenameBlockGenerator

@yjung-anl yjung-anl added P: normal A defect affecting operation with a low possibility of significantly affects. T: defect An anomaly, which is anything that deviates from expectations. labels Apr 29, 2021
@yjung-anl
Copy link
Contributor Author

Already Fixed this bug in RenameBlockGenerator and will make PR soon.

@loganharbour
Copy link
Member

Are you creating temporary blocks when a from block exists in the to blocks? That's how it should be done here; we just did the same with RenameBoundaryGenerator.

@yjung-anl
Copy link
Contributor Author

yjung-anl commented Apr 29, 2021

@loganharbour
In case of creating multiple pins and merging them together, I renamed the block IDs after each pin creation for assigning for different materials in the final merged mesh.

This is the code section of replacing the block id in RenameBlockGenerator

  for (const auto & elem : mesh->active_element_ptr_range())
    for (unsigned i = 0; i < _old_block_id.size(); ++i)
      if (elem->subdomain_id() == _old_block_id[i])
        elem->subdomain_id() = _new_block_id[i];

I think break should be inserted as:

  for (const auto & elem : mesh->active_element_ptr_range())
    for (unsigned i = 0; i < _old_block_id.size(); ++i)
      if (elem->subdomain_id() == _old_block_id[i])
      {
        elem->subdomain_id() = _new_block_id[i];
        break;
      }

Otherwise, block 1 is replaced to 3 first then, replaced again to 1 due to new_block_id setting.

@loganharbour
Copy link
Member

I'm afraid that won't work in the case of when boundary names are input. We need to keep track of the underlying ID, and then create temp blocks. For example:

We have the following blocks

  • exterior (0)
  • interior (1)
  • something_else (2)

And the user wants this

old_block_id = '0 1 2'
new_block_id = '1 2 0'

What we would do is first notice that 0 exists within the new IDs, so we would create a temp '3'. Then, we would do
0 -> 3, 1 -> 2, 2-> 0, 3-> 1.

This makes it independent of ordering. In addition, I'd like to simplify the parameters and only have a single parameter old_block and new_block. I did the same for RenameBoundaryGenerator. I can actually take a look at this right now, I suspect it's a little more work than you expected. Let me know.

@yjung-anl
Copy link
Contributor Author

Now I see your point. For avoiding the issue, it would be better to define a temporary block id rather than doing the way I did. Good to know. Thank you!

@loganharbour
Copy link
Member

I just rewrote RenameBoundaryGenerator for this similar behavior, and I'm in a good place to rework RenameBlockGenerator in the same sense. We'd also like to remove the {}_id and {}_name parameters and let the user supply both IDs and names at the same time. If you have something that works for you for now, how about you use that and I'll work on reworking RenameBlockGenerator this morning?

@yjung-anl
Copy link
Contributor Author

I think your modification would work for my cases - providing both Names an IDs at the same time. Thank you for very quick response!

loganharbour added a commit to loganharbour/moose that referenced this issue Apr 29, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 29, 2021
- Remove _id and _name params to the single old_block and new_block params
- Allow merging that is independent of ordering

refs idaholab#17710
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 29, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 29, 2021
@loganharbour
Copy link
Member

@yjung-anl, see #17711 for the new capability

loganharbour added a commit to loganharbour/moose that referenced this issue Apr 29, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 29, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 29, 2021
- Remove _id and _name params to the single old_block and new_block params
- Allow merging that is independent of ordering

refs idaholab#17710
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 29, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 29, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 29, 2021
@yjung-anl
Copy link
Contributor Author

I checked your updates. It looks good to me!

loganharbour added a commit to loganharbour/moose that referenced this issue Apr 30, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 30, 2021
- Remove _id and _name params to the single old_block and new_block params
- Allow merging that is independent of ordering

refs idaholab#17710
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 30, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 30, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 30, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 30, 2021
- Remove _id and _name params to the single old_block and new_block params
- Allow merging that is independent of ordering

refs idaholab#17710
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 30, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Apr 30, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue May 3, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue May 3, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue May 3, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 15, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 15, 2021
- Remove _id and _name params to the single old_block and new_block params
- Allow merging that is independent of ordering

refs idaholab#17710
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 15, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 15, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 15, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
- Remove _id and _name params to the single old_block and new_block params
- Allow merging that is independent of ordering

refs idaholab#17710
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
loganharbour added a commit to loganharbour/moose that referenced this issue Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Framework P: normal A defect affecting operation with a low possibility of significantly affects. T: defect An anomaly, which is anything that deviates from expectations.
Projects
None yet
2 participants