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

meshio4 #1093

Merged
merged 13 commits into from
Oct 7, 2020
Merged

meshio4 #1093

merged 13 commits into from
Oct 7, 2020

Conversation

gdmcbain
Copy link
Contributor

Fixes #1092.

(This is mostly to see how big a change is required and to make use of the continuous integration to test against more versions of Python etc. than I have installed.)

@sdrave sdrave requested a review from renefritze September 16, 2020 10:04
@sdrave sdrave added the pr:change Change in existing functionality label Sep 16, 2020
@sdrave sdrave added this to the 2020.2 milestone Sep 16, 2020
@sdrave
Copy link
Member

sdrave commented Sep 16, 2020

@renefritze, somehow this PR did not trigger a CI build. Maybe related to the recent Gitlab update?

@gdmcbain gdmcbain mentioned this pull request Sep 16, 2020
@renefritze
Copy link
Member

I hope the general CI issue is fixed now.

CI checks if dependencies.py and the requirements* files match. If you execute ./dependencies.py (or just make) and commit the update, then CI should continue.

@renefritze
Copy link
Member

Sorry I didn't think of that right away, but I'll have to update the CI images so that the pypi mirror serves the newer meshio.

@renefritze
Copy link
Member

Sorry for the spam, @gdmcbain, the bot isn't quite doing what it's supposed to and needs some reprogramming still.
Meanwhile, docker images are updating here.

@renefritze renefritze self-assigned this Sep 24, 2020
@pymor pymor deleted a comment from pymor-lab-hub-bridge bot Sep 24, 2020
@pymor pymor deleted a comment from pymor-lab-hub-bridge bot Sep 24, 2020
@pymor pymor deleted a comment from pymor-lab-hub-bridge bot Sep 24, 2020
@renefritze
Copy link
Member

There's now an actual pytest error happening: https://zivgitlab.uni-muenster.de/pymor/pymor/-/jobs/423244#L966

@gdmcbain
Copy link
Contributor Author

gdmcbain commented Sep 25, 2020

There's now an actual pytest error happening: https://zivgitlab.uni-muenster.de/pymor/pymor/-/jobs/423244#L966

Yes. I see that it's in

https://github.com/nschloe/meshio/blob/d467cb8a49e218b7bee899f59def3c3f898b3133/meshio/gmsh/_gmsh41.py#L117

I'm not getting the error here though with

python src/pymordemos/elliptic_unstructured.py 6 16 1e-1

Perhaps as without an explicit installation of Gmsh, pyMOR's picking up /usr/bin/gmsh which is 4.4.1 for Ubuntu 20.04.1 LTS. If (as usual) I do

pip install gmsh-sdk

I get 4.6.0, and no error with that either.

At https://zivgitlab.uni-muenster.de/pymor/pymor/-/jobs/423244#L981, it looks like Gmsh 4.1.3. This was a shortlived buggy version of Gmsh, as noted in nschloe/meshio#797:

That's a bug indeed, but mind you not many people take interest in this particular version of gmsh. It uses an "intermediate" format that was fixed on version after. You might want to upgrade.

Any chance of upgrading from Gmsh 4.1.3?

@sdrave
Copy link
Member

sdrave commented Sep 25, 2020

I get 4.6.0, and no error with that either.

At https://zivgitlab.uni-muenster.de/pymor/pymor/-/jobs/423244#L981, it looks like Gmsh 4.1.3. This was a shortlived buggy version of Gmsh, as noted in nschloe/meshio#797:

That's a bug indeed, but mind you not many people take interest in this particular version of gmsh. It uses an "intermediate" format that was fixed on version after. You might want to upgrade.

Any chance of upgrading from Gmsh 4.1.3?

Debian stable, on which our CI images are based if I remember correctly, has 4.1.5+really4.1.3+ds1-1. On my Debian machine I get the same error locally. However, I would be fine with adding gmsh to our pymor[full] requirements (gmsh-sdk is deprecated). It has wheels for all relevant platforms.

@renefritze
Copy link
Member

I agree, adding the python package that gets us the right gmsh would be a better solution than to get a newer gmsh binary into our debian stable based images.

I'll update the images again to make gmsh available.

@renefritze
Copy link
Member

I realized I missed some stuff in my previous commit @gdmcbain, so I've added a fixup commit which we should squash before the merge.

@renefritze
Copy link
Member

It seems we're now having trouble calling the gmsh binary/script installed from the gmsh python package. I can look into that after the pyMOR course is over.

@renefritze
Copy link
Member

The CI images needed some more Xorg packages that had been installed as a gmsh apt package before. It'll take a bit to get those updates out and then I'll restart the gitlab pipelines.

@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@5d99181). Click here to learn what that means.
The diff coverage is 87.50%.

Impacted Files Coverage Δ
src/pymor/discretizers/builtin/grids/gmsh.py 87.50% <87.50%> (ø)

@renefritze
Copy link
Member

Tests are looking good now. If there's nothing else @gdmcbain @pymor/pymor-devs , I'll merge this in a day.

@gdmcbain
Copy link
Contributor Author

gdmcbain commented Oct 7, 2020

No, that looks good. Thanks very much for following this through; there were a few more things required than I had envisaged.

@renefritze
Copy link
Member

No, that looks good.

Great. This is now on track to merge. Thank you again for your work, @gdmcbain !

@renefritze renefritze merged commit bdcd2ca into pymor:master Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:change Change in existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

meshio 4
3 participants