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

Support pygmsh 7.0.0 in the examples #484

Closed
kinnala opened this issue Sep 30, 2020 · 24 comments · Fixed by #489
Closed

Support pygmsh 7.0.0 in the examples #484

kinnala opened this issue Sep 30, 2020 · 24 comments · Fixed by #489

Comments

@kinnala
Copy link
Owner

kinnala commented Sep 30, 2020

I was unable to run pygmsh 7.0.0 on my laptop so I didn't do this while looking into #482 .

I suppose now is also the time to do #333 because otherwise I might be unable to run the tests locally unless they support both 6.x and 7.x.

@kinnala
Copy link
Owner Author

kinnala commented Sep 30, 2020

I think this package gmsh is a good example of why you shouldn't distribute binaries in a PyPI package unless you want to put some effort into making it cross-platform and cross-distribution: https://gitlab.onelab.info/gmsh/gmsh/-/tree/master/utils/pypi/gmsh

It should be a manylinux wheel instead.

@ahojukka5
Copy link
Contributor

If you need to just read gmsh file formats, consider using gmshparser. It has zero dependencies (the point of the package is not to introduce any complicated dependencies to FEM projects).

https://github.com/ahojukka5/gmshparser

@gdmcbain
Copy link
Contributor

gdmcbain commented Sep 30, 2020

It should be a manylinux wheel instead.

Oh, O. K.

I had been relying routinely upon pip install gmsh-sdk to get a working recent Gmsh on Windows (especially during the lockdown) and then as it was so easy even back on Linux, until I was informed last week in pymor/pymor#1093 that it had been deprecated (in favour of gmsh or gmsh-dev). Now that pygmsh depends on gmsh and not gmsh-sdk, I can't get pygmsh to install at all on Windows: nschloe/pygmsh#370. This puzzled me a bit though as when I looked at the code for pip install gmsh, it seemed identical to gmsh-sdk, having been copied from it.

@gdmcbain
Copy link
Contributor

If you need to just read gmsh file formats

pygmsh doesn't read gmsh file formats. meshio does that. pygmsh generates Gmsh GEO code.

@gdmcbain
Copy link
Contributor

gdmcbain commented Oct 6, 2020

There's been an upstream change in the PyPI package gmsh; pip install gmsh seems to work O. K. now (Ubuntu 20.04.1 LTS and MS-Windows 10). It's a little slow on the step ‘Building wheel for gmsh (setup.py)’ but gets there in the end. Similarly pip install pygmsh works, bringing in pygmsh-7.0.0 and gmsh-4.6.0.post3.

(Not that I'm pushing to unpin pygmsh just yet.)

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

The issue I have with gmsh (the PyPI package) is that it distributes a precompiled binary (or binaries, dunno) that dynamically links to fairly recent versions of, e.g., X11-related libraries. In fact, the versions expected by gmsh (the PyPI package) are so new that I cannot find those from the package repositories of my distribution of choice. (I did not verify or look into it in detail, this is just how it appears to me.) I still run into same issues after updating to gmsh-4.6.0.post3.

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

Let me add that this is obviously partially my own problem for running such an esoteric distribution. But the way I'd like it solved is to containerize the test suite so it doesn't matter which OS you are using when developing.

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

I'm making a proof-of-concept Github Action which uses a container in https://github.com/kinnala/scikit-fem-docker-action

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

Ok, here you can see the same issue that I'm bumping to. I expect this to be fixed by installing some Ubuntu-specific library version: https://github.com/kinnala/scikit-fem/runs/1213945008?check_suite_focus=true

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

Next in line is libxinerama. This is actually the library which I wasn't able to find a new enough version from NixOS package repositories.

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

It would be really nice to have these additional dependencies listed somewhere, e.g., here https://pypi.org/project/gmsh/

@gdmcbain
Copy link
Contributor

gdmcbain commented Oct 6, 2020

I was wondering what the 'esoteric' distro was. NixOS is an appealing choice. I've deferred it for the future, thus far, myself, but for reproducibility it sounds very good.

I'm very far from accepting pygmsh 7. It seems to have thrown out a lot that I regard as essential for finite element work; e.g. physical entities.

1 similar comment
@gdmcbain
Copy link
Contributor

gdmcbain commented Oct 6, 2020

I was wondering what the 'esoteric' distro was. NixOS is an appealing choice. I've deferred it for the future, thus far, myself, but for reproducibility it sounds very good.

I'm very far from accepting pygmsh 7. It seems to have thrown out a lot that I regard as essential for finite element work; e.g. physical entities.

@gdmcbain
Copy link
Contributor

gdmcbain commented Oct 6, 2020

I haven't done too much work in containers, so this suggestion might be off the mark, but would it make sense for the integration tests #474 to use multiple containers: say a nice strictly reproducible NixOS container for the scikit-fem core but then whatever else might be required for the externals; e.g. the latest Ubuntu LTS for Gmsh. Multiple containers can communicate via the file system, can't they?

An advantage of pygmsh<7 is that it can be used to generate GEO code without Gmsh present at all. That GEO code can conceptually then be submitted to a Gmsh server wherever (or invoked from another container on the same system) and a MSH 4.1 mesh file received in return. That can then be loaded with skfem.io.meshio.from_file. The difference with pygmsh 7 is the pygmsh code has to import the Gmsh Python API #180 to work at all.

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

I haven't done too much work in containers, so this suggestion might be off the mark, but would it make sense for the integration tests #474 to use multiple containers: say a nice strictly reproducible NixOS container for the scikit-fem core but then whatever else might be required for the externals; e.g. the latest Ubuntu LTS for Gmsh. Multiple containers can communicate via the file system, can't they?

Yeah, that is a good idea. I'll try to make it extensible so that we can easily add more distributions, Python versions, package versions etc. if needed.

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

An advantage of pygmsh<7 is that it can be used to generate GEO code without Gmsh present at all. That GEO code can conceptually then be submitted to a Gmsh server wherever (or invoked from another container on the same system) and a MSH 4.1 mesh file received in return. That can then be loaded with skfem.io.meshio.from_file. The difference with pygmsh 7 is the pygmsh code has to import the Gmsh Python API #180 to work at all.

Wouldn't that be fixed by a simple try-except construction in the import?

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

This is actually really hard to support both 7 and 6 in a single code because now Geometry is a context manager to be used inside a with-statement. I need to learn some Python.

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

Alright, now I'm running into missing features and unable to fix it myself.

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

Maybe @gdmcbain you have some hints? You can see the progress in #489 . The issue with example 28 is that it doesn't seem to parse the subdomains properly, no idea what causes that. Example 32 is working. Example 35 results in duplicate nodes, no idea what causes that.

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

Decided to remove pygmsh from example 35 because the porting was starting to be too complicated.

@kinnala
Copy link
Owner Author

kinnala commented Oct 6, 2020

Also temporarily removed mesh generation from ex28 to get the testing stuff merged. Feel free to reintroduce it @gdmcbain when you have time to learn pygmsh 7.

@gdmcbain
Copy link
Contributor

gdmcbain commented Oct 6, 2020

Feel free to reintroduce it @gdmcbain when you have time to learn pygmsh 7.

I have little motivation to learn pygmsh 7. It breaks a lot of essentials (physical entities nschloe/meshio#373 nschloe/meshio#378 has been dropped).

Maybe @gdmcbain you have some hints?

  1. Pin pygmsh<7.
  2. Monitor progress on pygmsh 7, 8, …. Should support for physical entities be reintroduced, consider updating to it.
  3. If additions to pygmsh==6.1.1 are required, fork it.

I note that other packages are dropping pygmsh in the wake of pygmsh 7; e.g. FEniCS/dolfinx#1145.

@gdmcbain
Copy link
Contributor

gdmcbain commented Oct 7, 2020

I see that ex28.make_mesh #489 isn't used. It doesn't work with pygmsh 7

https://github.com/FEniCS/dolfinx/pull/1145

raises

warnings.warn("Unable to load tagged boundaries/subdomains.")

And then mesh.boundaries is None so the boundary-value problem can't be formulated. This is the broken support for physical entities in pygmsh 7. Thank you for trying. It might be better to suppress the nonworking pygmsh 7 code-path unless there's action upstream.

@gdmcbain
Copy link
Contributor

gdmcbain commented Oct 7, 2020

The case of ex32, the ellipsoid, is better; that doesn't need physical entities since there is only one subdomain and it's not a mixed boundary value problem.

Since the introduction of the very clever MeshTri.init_circle #475 #477, I've been wondering whether there's an analogous technique for ellipsoids.

Whereas storing static meshes in JSON is reasonable in two dimensions, it does seem awfully wasteful in three, should an algorithm be available. I wonder whether ngsimple would do.

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 a pull request may close this issue.

3 participants