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 trianglemesh custom_dataset bug #1041

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

tylerflex
Copy link
Collaborator

@tylerflex tylerflex commented Jul 31, 2023

The custom_datasets had the wrong logic. It was looping through geometry group items but not checking if they were TriangleMesh before grabbing the datasets. Was breaking Anderson localization (and probably metalens) notebooks.

For now, omitting changelog item since this bug was introduced in this commit and doesn't affect develop.

@tylerflex tylerflex requested a review from caseyflex July 31, 2023 17:31
@tylerflex tylerflex force-pushed the tyler/fix/custom_triangle_mesh branch from b98dc8f to 22ccd38 Compare July 31, 2023 17:38
Copy link
Contributor

@caseyflex caseyflex left a comment

Choose a reason for hiding this comment

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

looks good, thanks for catching and fixing this. Is there a test we can add to make sure the custom_datasets are working correctly? I guess in this case it would error whenever it was called for a GeometryGroup containing geometries that weren't TriangleMesh.

@tylerflex
Copy link
Collaborator Author

Fair enough, I added a test but I'm seeing strange behavior. seems the custom_datasets is not what I'd expect. With two TriangleMesh geometries, I see 4 datasets, each of which is a tuple and two of which just contain ('type', 'TriangleMeshDataset'). Could you look into what's going on here? I'm kind of fuzzy on the workings of the triangle mesh.

@caseyflex
Copy link
Contributor

Fair enough, I added a test but I'm seeing strange behavior. seems the custom_datasets is not what I'd expect. With two TriangleMesh geometries, I see 4 datasets, each of which is a tuple and two of which just contain ('type', 'TriangleMeshDataset'). Could you look into what's going on here? I'm kind of fuzzy on the workings of the triangle mesh.

I think changing the line in the custom_datasets to datasets_geometry += [geometry.mesh_dataset] fixes this

@tylerflex tylerflex force-pushed the tyler/fix/custom_triangle_mesh branch from 6b9a09a to 428d694 Compare July 31, 2023 18:14
@tylerflex
Copy link
Collaborator Author

Yea that does seem to do it, although I guess it's also wrong in develop then?

@tylerflex
Copy link
Collaborator Author

oh wait, I think it's correct in develop actually since it's inside of a list comprehension.

@caseyflex
Copy link
Contributor

caseyflex commented Jul 31, 2023 via email

@tylerflex tylerflex force-pushed the tyler/fix/custom_triangle_mesh branch from 428d694 to 1523dcb Compare July 31, 2023 18:23
@tylerflex tylerflex force-pushed the tyler/fix/custom_triangle_mesh branch from 1523dcb to 3667deb Compare July 31, 2023 18:30
@tylerflex tylerflex merged commit 8fb80b5 into pre/2.4 Jul 31, 2023
11 checks passed
@tylerflex tylerflex deleted the tyler/fix/custom_triangle_mesh branch July 31, 2023 18:59
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