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

Make Mesh into a dataclass #400

Merged
merged 6 commits into from
Apr 26, 2024
Merged

Make Mesh into a dataclass #400

merged 6 commits into from
Apr 26, 2024

Conversation

alexfikl
Copy link
Collaborator

@alexfikl alexfikl commented Apr 7, 2024

This starts making Mesh into a dataclass. Right now it's mostly just porting away from pytools.Record, since it's hard to make it a clean dataclass while being backwards compatible.

The main additions are:

  • a make_mesh factory function that does all the work of the Mesh constructor.
  • a check_mesh_consistency function that does all the checking. This seemed useful to make public, since the dataclass version won't do any checking on replace(mesh, ...) and it may still be wanted sometimes?
  • get_nodal_adjacency and get_facial_adjacency_groups to replace the property versions.

The nodal / facial adjacency attributes were very annoying. Currently it overwrites __getattribute__ to make it behave a bit like before. Let me know if you have some better ideas on how to make that work..

This is still a WIP, but I had it lying around for a while, so might as well. Things still to do:

  • Run the CI and make everything actually works. Currently it works on my machine
  • Left a few remarks / questions below that should be looked at.
  • Port at least the internal uses of the various functions to avoid deprecation warnings.
  • Not quite sure why the documentation build is failing? It seems to dislike the tikz stuff, but that shouldn't have changed..

meshmode/mesh/__init__.py Outdated Show resolved Hide resolved
meshmode/mesh/__init__.py Outdated Show resolved Hide resolved
meshmode/mesh/__init__.py Outdated Show resolved Hide resolved
meshmode/mesh/__init__.py Outdated Show resolved Hide resolved
@alexfikl alexfikl force-pushed the mesh-dataclass branch 2 times, most recently from a686130 to 83183c9 Compare April 8, 2024 06:01
@alexfikl alexfikl marked this pull request as ready for review April 8, 2024 15:37
@alexfikl alexfikl force-pushed the mesh-dataclass branch 2 times, most recently from df928ae to 12a8c99 Compare April 16, 2024 06:44
meshmode/mesh/__init__.py Outdated Show resolved Hide resolved
meshmode/mesh/__init__.py Outdated Show resolved Hide resolved
@alexfikl alexfikl force-pushed the mesh-dataclass branch 5 times, most recently from 36ef3d4 to 19286d4 Compare April 25, 2024 13:13
@alexfikl
Copy link
Collaborator Author

@inducer I think all of my initial questions are solved now, so this is ready for a proper review.

Port at least the internal uses of the various functions to avoid deprecation warnings.

I'm thinking of leaving this for a followup PR, since this is sufficiently big?

@alexfikl alexfikl force-pushed the mesh-dataclass branch 2 times, most recently from 11cc84f to 7aa1897 Compare April 25, 2024 13:25
meshmode/mesh/__init__.py Outdated Show resolved Hide resolved
meshmode/mesh/__init__.py Show resolved Hide resolved
meshmode/mesh/__init__.py Outdated Show resolved Hide resolved
@inducer
Copy link
Owner

inducer commented Apr 26, 2024

Pushed a commit that ports the internal uses of Mesh(). Mostly a sed job.

@inducer inducer force-pushed the mesh-dataclass branch 2 times, most recently from b294d8b to 8293f58 Compare April 26, 2024 16:51
@inducer
Copy link
Owner

inducer commented Apr 26, 2024

I guess I sort of understand why you might have changed the make_mesh arguments to groups, *, vertices, but I don't think it's worth the headache. 99% of meshes have vertices, and the 1% that don't can just pass None for that first argument.

@inducer inducer force-pushed the mesh-dataclass branch 2 times, most recently from d39c715 to 232e0a8 Compare April 26, 2024 17:00
@alexfikl
Copy link
Collaborator Author

I guess I sort of understand why you might have changed the make_mesh arguments to groups, *, vertices, but I don't think it's worth the headache. 99% of meshes have vertices, and the 1% that don't can just pass None for that first argument.

That's fair enough. It definitely makes the porting easier 😁 Thanks for helping with that and fixing all the issues!

@inducer inducer enabled auto-merge (rebase) April 26, 2024 19:30
@inducer
Copy link
Owner

inducer commented Apr 26, 2024

Thanks for getting this most of the way!

@inducer inducer merged commit 694775f into inducer:main Apr 26, 2024
12 checks passed
@alexfikl alexfikl deleted the mesh-dataclass branch April 27, 2024 04:30
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