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

Weaken mesh check in make_same_mesh_connection #359

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

majosm
Copy link
Collaborator

@majosm majosm commented Nov 1, 2022

Weakens the mesh check a bit to fix #358. Also adds an object identity check to the mesh __eq__ so it doesn't need to compare all the attributes if the two mesh objects are actually the same one.

@majosm majosm marked this pull request as ready for review November 3, 2022 19:10
@majosm majosm requested a review from inducer November 3, 2022 19:11
@inducer
Copy link
Owner

inducer commented Nov 6, 2022

I don't know that I love this, because pretty much by definition, we'll have redundant meshes flying around, which seems avoidable. First of all, could you remind me where those come from? (Restart or something?) And do you think it'd be plausible to replace those two meshes with one "canonical" copy after determining that they're equal? I wouldn't be opposed to adding tools to help with that.

@majosm
Copy link
Collaborator Author

majosm commented Nov 7, 2022

I don't know that I love this, because pretty much by definition, we'll have redundant meshes flying around, which seems avoidable. First of all, could you remind me where those come from? (Restart or something?) And do you think it'd be plausible to replace those two meshes with one "canonical" copy after determining that they're equal? I wouldn't be opposed to adding tools to help with that.

It comes from here in inducer/grudge#285, where we promote the "part ID"s (rank, volume, or both depending on the problem) in the incoming mesh's adjacency data to a single data structure PartID. So each time a dcoll is created it makes a new mesh object. (We have two dcolls temporarily when we restart using a different order than what was stored in the restart file.) We could maybe memoize that function, but a mesh seems like kind of a heavyweight thing to memoize?

@inducer
Copy link
Owner

inducer commented Nov 8, 2022

Could you explain the reason for that normalization step? Why is it needed? (I'm not suggesting that it isn't, I'm just not currently understanding.)

@majosm
Copy link
Collaborator Author

majosm commented Nov 8, 2022

That was added to provide a uniform way of retrieving the neighbor parts' rank/volume for all of the possible mesh configurations (single/multiple volume, distributed/non-distributed). It was suggested as an alternative to the way I had been doing it previously with the part_id_helpers (see inducer/grudge#239, specifically inducer/grudge@09bac09).

@inducer
Copy link
Owner

inducer commented Nov 9, 2022

Still sort of puzzled, sorry. Let's talk about it during our meeting tomorrow.

@inducer
Copy link
Owner

inducer commented Nov 11, 2022

As discussed during meeting: We'll leave this open, but for now the creation of duplicate meshes seems avoidable by checking if the normalization step is a no-op.

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.

Mesh check in make_same_mesh_connection too strict?
2 participants