Skip to content

Add region tags#129

Closed
majosm wants to merge 10 commits into
inducer:mainfrom
majosm:region-tags
Closed

Add region tags#129
majosm wants to merge 10 commits into
inducer:mainfrom
majosm:region-tags

Conversation

@majosm

@majosm majosm commented Mar 11, 2021

Copy link
Copy Markdown
Collaborator

@inducer What would you say to doing something like this to handle volume tagging for geometry-based initialization (and later multi-domain)?

cc @dshtey2

@majosm majosm force-pushed the region-tags branch 2 times, most recently from 1b6c746 to c52e661 Compare March 11, 2021 01:15

@inducer inducer left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Some thoughts below.

Comment thread meshmode/mesh/__init__.py Outdated
Comment thread meshmode/mesh/__init__.py

Elements of :attr:`boundary_tags` that do not cover any
part of the boundary will not be keys in this dictionary.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is this information preserved somewhere?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

As far as I could tell, this wasn't actually true... The code in the mesh constructor didn't check whether the tags were empty.

Comment thread meshmode/mesh/__init__.py Outdated
return boundary_tags


def index_tags(tags):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
def index_tags(tags):
def tag_to_index_dict(tags):

?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Should I verbify it? get_tag_to_index_dict? Or just tag_to_index_dict?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

🤷 Not sure. Be consistent with surrounding code? :)

Comment thread meshmode/mesh/__init__.py

.. attribute:: region_tags

A list of region tag identifiers. :class:`RTAG_ALL` is guaranteed to exist.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

To what extent are empty tags supposed to be included? (Document this)

Comment thread meshmode/mesh/__init__.py
(Note that element groups are not necessarily geometrically contiguous
like the figure may suggest.)

.. attribute:: region_tags

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

volume_tags? volume_region_tags? I'm not hating region_tags, I'm just exploring altenatives.

Comment thread meshmode/mesh/__init__.py Outdated
return boundary_tags


def index_tags(tags):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Does this require that tags is narrowed to the non-empty ones first?

Comment thread meshmode/mesh/__init__.py Outdated
# {{{ is_region_tag_empty

def is_region_tag_empty(mesh, region_tag):
"""Return *True* if the corresponding region tag does not occur as part of

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
"""Return *True* if the corresponding region tag does not occur as part of
""":returns: *True* if the corresponding region tag does not occur as part of

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

pydocstyle didn't like this, FWIW. (Not sure if we're following its conventions here too or just in mirgecom.)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Nah, I find pydocstyle way too rigid to use everywhere.

Comment thread meshmode/mesh/__init__.py

*(dim, nunit_nodes)*

.. attribute:: regions

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should this live with the element group or the mesh? (I'm not sure, but I'm kind of leaning mesh.)

Comment thread meshmode/mesh/__init__.py Outdated
Comment on lines +198 to +199
An array *(nelements)*, with the bits of ``regions[i]`` indicating the
mesh regions that contain element ``i``.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If this is allowed to be None, document that fact. (here or wherever it ends up)

@majosm majosm mentioned this pull request Jan 24, 2022
@majosm

majosm commented Apr 12, 2022

Copy link
Copy Markdown
Collaborator Author

Superseded by #320.

@majosm majosm closed this Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants