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

more streamlined handling of point and cell data #704

Merged
merged 8 commits into from
Feb 17, 2020

Conversation

nschloe
Copy link
Owner

@nschloe nschloe commented Feb 17, 2020

For mesh formats that can only write one point/cell data array of int data, unconditionally pick the first one found and warn if there are more candidates.

@nschloe nschloe merged commit efd606f into master Feb 17, 2020
@nschloe nschloe deleted the unify-point-cell-data branch February 17, 2020 23:24
Copy link
Contributor

@tianyikillua tianyikillua left a comment

Choose a reason for hiding this comment

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

Yes I see your approach here. We all assume that there may exist several int dtyped point or cell data in the mesh (say a user defined a point array as the number of cells that each point is connected to), and you use the first one as the tag for interoperability.

Actually I still prefer maintaining a list of these official tag data for each format, since explicit is better than implicit

  • Either really provide a global list of these keys
  • Either point mesh.point_tags to a particular data array

meshio/_mesh.py Show resolved Hide resolved
@nschloe
Copy link
Owner Author

nschloe commented Feb 18, 2020

Actually I still prefer maintaining a list of these official tag data for each format, since explicit is better than implicit

But that is implicit, too. The user never sees the "official tag list", it's a member of some function or class. When she writes a file, it's completely unclear why one set has been selected instead of another.

The way we have it right now, at least we have a warning about it. If the user thinks it's the wrong cell_data written out, she needs to provide only one cell_data item. That's as explicit as it gets.

@tianyikillua
Copy link
Contributor

Okay. So I suppose currently for practical use we must save "gmsh:physical" first in the cell_data?

Do you plan to add a test file for testing interoperability?

Also the MED format needs to be included. It's by far the most comprehensive format: point / cell data as well as point / cell tags are supported.

What about exposing the mesh.*_tags attributes?

@nschloe
Copy link
Owner Author

nschloe commented Feb 18, 2020

So I suppose currently for practical use we must save "gmsh:physical" first in the cell_data?

If you want "gmsh:physical" to be saved, then I suggest this to be the only cell_data you pass.

Do you plan to add a test file for testing interoperability?

We can do that.

What about exposing the mesh.*_tags attributes?

Do you mean *_sets? (Otherwise I don't know what you mean.)

@gdmcbain
Copy link
Contributor

gdmcbain commented Feb 18, 2020

So I suppose currently for practical use we must save "gmsh:physical" first in the cell_data?

Actually for practical use downstream when reading Gmsh MSH 4.1, I recommend users ignore "gmsh:physical" and instead read .cell_sets, or rsther .cell_sets_dict.
I don't know how to deprecate the old behaviour; it's part of the value of an attribute.

The complication is Gmsh version.. This is only for reading MSH 4.1, which is genersted by default by Gmsh 4. Older Gmsh can't generate MSH 4.1, only MSH 2.2. I believe Gmsh 3 is still distributed by a number of Linux package managers.

It would be easy enough to populate the .cell_sets` on reading any format with a simple cell-colouring (Gmsh's MSH 2.2, MEDIT, …), just with a bit of redundancy.

But what about attempting this in the property .cell_sets_dict? Should this dig into .cell_data for int-valued colourings?

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.

3 participants