Skip to content

Conversation

@capitalaslash
Copy link
Contributor

@capitalaslash capitalaslash commented Feb 17, 2021

Closes #397.

@capitalaslash
Copy link
Contributor Author

I not super-happy with the way I fixed this issue, but it works for my applications.
I based it on the way meshio sets up the physical tags when reading a msh file.

@capitalaslash
Copy link
Contributor Author

Do not merge. The geometrical tag must also be set to get this properly working, I am adding it.

@nschloe nschloe marked this pull request as draft February 17, 2021 14:54
@capitalaslash capitalaslash force-pushed the fix_physical_tags_write_mesh branch from 12a4d83 to 8abc3cb Compare February 17, 2021 16:11
@capitalaslash capitalaslash marked this pull request as ready for review February 17, 2021 16:28
@capitalaslash
Copy link
Contributor Author

The branch is ready for review.
The geometrical tag is set reusing the same code that is used for the physical tag, so it is set only where there are physical tags set.
When no physical tags are set, those 2 tags will not be present (meshio will print 0s in msh format, as they are always present in the file format).
I believe the default behavior in gmsh when no physical tags are present is to set them to the geometrical one? Not sure it would be useful.

Repository owner deleted a comment from capitalaslash Feb 18, 2021
@nschloe
Copy link
Owner

nschloe commented Feb 18, 2021

I not super-happy with the way I fixed this issue,

Neither. I don't understand why this PR switches away from cell_sets to cell_data + field_data. I've now fixed the corresponding bug simply by deprecating empty labels. Is there anything else in the PR that's worth merging?

@capitalaslash
Copy link
Contributor Author

I not super-happy with the way I fixed this issue,

Neither. I don't understand why this PR switches away from cell_sets to cell_data + field_data. I've now fixed the corresponding bug simply by deprecating empty labels. Is there anything else in the PR that's worth merging?

The problem goes beyond the referenced bug. A mesh constructed in pygmsh cannot be printed in gmsh41 format, spitting out errors about

  File "/home/aslash/.local/lib/python3.9/site-packages/meshio/gmsh/_gmsh41.py", line 610, in _write_nodes
    raise WriteError(
meshio._exceptions.WriteError: Specify entity information to deal with more than one cell type

nor in gmsh22 format, since that one would not use cell_sets.
Other formats (such as vtu) ignore cell_sets.

My problem is simple: I want to print out a mesh that I can open in gmsh directly.

So I see 2 ways out: meshio._gmsh22 adds support for physical tags stored as cell_sets or pygmsh creates a mesh that can be fed to meshio._gmsh41.
Let me know what a suitable strategy would be to get over this problem.

@nschloe
Copy link
Owner

nschloe commented Feb 18, 2021

My problem is simple: I want to print out a mesh that I can open in gmsh directly.

Yeah, that should be made possible. Note that you can always fall back to gmsh like

geom.generate_mesh()
gmsh.write("mesh3D.msh")

I'm wondering how gmsh itself writes the file, and if the issue should not be fixed in meshio after all.

@capitalaslash
Copy link
Contributor Author

geom.generate_mesh()
gmsh.write("mesh3D.msh")

That works. Why not add that to the pygmsh interface?

I'm wondering how gmsh itself writes the file, and if the issue should not be fixed in meshio after all.

Physical and geometrical (in pygmsh, elementary in gmsh) tags in gmsh are special: they are always present, that is why this branch moves that information to cell_data.
For any gmsh compatible mesh those flags must be there. If the user did not specify the physical tags, they correspond to the elementary ones.
This information cannot be generated at the meshio stage since there is no gmsh.model available there, so they must be stored somewhere. cell_sets can be adequate for physical tags, but they are awkward for elementary tags: there is always a geometrical object behind a mesh element so they cannot be defined on a partition of the mesh as we can do for the physical tags.

I'll point out that even meshio._gmsh41 when writing expects that that information is stored in cell_data

https://github.com/nschloe/meshio/blob/b5229a8181745bd63d819810a7f0dbcb4ace224a/meshio/gmsh/_gmsh41.py#L338

@nschloe
Copy link
Owner

nschloe commented Feb 18, 2021

Why not add that to the pygmsh interface?

Yeah, why not actually. Just added it as pygmsh.write() (#426).

@nschloe
Copy link
Owner

nschloe commented Feb 18, 2021

Physical and geometrical (in pygmsh, elementary in gmsh) tags in gmsh are special: they are always present, that is why this branch moves that information to cell_data.

A yes, right. Every cell is guaranteed to have both of those tags so we can treat it as a function (aka cell_data), not a cell_set.

I can't quite read the PR, still. Perhaps it could be made clearer, I don't know. For example, instead of

 cell_data["gmsh:physical"] = [
    numpy.array([0 for _ in range(len(cells[i][1]))]) for i in range(len(cells))
)

one could go

 cell_data["gmsh:physical"] = [
    numpy.array([0 for _ in range(len(cell[1]))]) for cell in cells
)

but I still don't understand what signficance cell[1] has, or why it's initialized with 0. Isn't it overridden afterwards? Perhaps not, who knows what values elem_tags - 1 can take.

Considering the gmsh error your describe, I think we need a "gmsh:dim_tags" point data entry.

@capitalaslash capitalaslash force-pushed the fix_physical_tags_write_mesh branch from 8abc3cb to edb9a8e Compare February 18, 2021 20:37
@capitalaslash
Copy link
Contributor Author

I can't quite read the PR, still. Perhaps it could be made clearer, I don't know. For example, instead of

 cell_data["gmsh:physical"] = [
    numpy.array([0 for _ in range(len(cells[i][1]))]) for i in range(len(cells))
)

one could go

 cell_data["gmsh:physical"] = [
    numpy.array([0 for _ in range(len(cell[1]))]) for cell in cells
)

I improved there with numpy.full.

but I still don't understand what signficance cell[1] has, or why it's initialized with 0. Isn't it overridden afterwards? Perhaps not, who knows what values elem_tags - 1 can take.

cell[1] stores the size of the group of elements. This guarantees that the array are properly sized, since cell_data must be complete as discussed above. Zero is an arbitrary initialization value (that gmsh does not use so it's easy when debugging).

After some thoughts, I believe the best way would be to always fill in a gmsh:geometrical cell_data based on the elementary tags. The gmsh:physical is then added only if the model has some physical tag, otherwise is the same as the first one, similarly to how gmsh fills them.

I will pursue this if you deem it mergeable.

Considering the gmsh error your describe, I think we need a "gmsh:dim_tags" point data entry.

I will add a separate bug report (and hopefully a merge request) about this.

@nschloe
Copy link
Owner

nschloe commented Feb 19, 2021

I improved there with numpy.full.

It should be zeros or empty I think.

I will pursue this if you deem it mergeable.

Let's first get the "gmsh:dim_tags" issue out of the way.

@capitalaslash capitalaslash force-pushed the fix_physical_tags_write_mesh branch from edb9a8e to b79f580 Compare February 19, 2021 13:17
@nschloe
Copy link
Owner

nschloe commented Oct 22, 2021

This might have been fixed by #486.

@capitalaslash
Copy link
Contributor Author

This might have been fixed by #486.

unfortunately not fixed yet.

@capitalaslash capitalaslash force-pushed the fix_physical_tags_write_mesh branch from b79f580 to ec0ee4f Compare October 22, 2021 11:52
@nschloe
Copy link
Owner

nschloe commented Oct 23, 2021

Okay, perhaps this becomes clearer with a test. Could you add one?

@capitalaslash
Copy link
Contributor Author

Okay, perhaps this becomes clearer with a test. Could you add one?

see #488

@nschloe
Copy link
Owner

nschloe commented Oct 24, 2021

Thanks for the test! We need one that fails before this PR, though, and passes with the PR.

@capitalaslash capitalaslash force-pushed the fix_physical_tags_write_mesh branch from ec0ee4f to 5f04e2d Compare October 24, 2021 14:36
@capitalaslash
Copy link
Contributor Author

Thanks for the test! We need one that fails before this PR, though, and passes with the PR.

test_physical.py should have failed with the other merge, but it does not in pytest, I'm trying to understand why...

@capitalaslash
Copy link
Contributor Author

my bad, I asserted the read mesh in the main function instead of the test one.
see #489

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.

physical groups

2 participants