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

TetGen 1.6, artifacts etc. #21

Closed
j-fu opened this issue Oct 12, 2020 · 13 comments
Closed

TetGen 1.6, artifacts etc. #21

j-fu opened this issue Oct 12, 2020 · 13 comments

Comments

@j-fu
Copy link
Member

j-fu commented Oct 12, 2020

Hi, there is a new TetGen version 1.6 available.
Also, I would like to enhance the API in order to get more information out (e.g. cell, facet region numbers).
My ultimate plan is to use TetGen in addition to Triangle in SimplexGridFactory.jl .
Also, transition to artifacts would be cool.
I would volunteer to work on this in the next couple of months if nobody else plans to do so.
How do you see this ?

@SimonDanisch
Copy link
Member

That'd be great :) Let me know if you have any questions!

@j-fu
Copy link
Member Author

j-fu commented Oct 13, 2020

Whats the best place to discuss them, in case ? I assume here ?

@SimonDanisch
Copy link
Member

Yeah why not ;)

@j-fu
Copy link
Member Author

j-fu commented Oct 13, 2020

Hi,
got the first test running with artifacts, for linux so far, see https://github.com/j-fu/YggBonsai/blob/master/T/TetGen/build_tarballs.jl

I don't expect problems with the other architectures.

The jll is temporarily registered in https://github.com/j-fu/PackageNursery.
Once we agree, I'll build for all architectures and make the PR to Yggdrasil, once it's there, I can make the PR to JuliaGeometry.

Some questions:

  • As for the c wrapper, I made a here script to create the file directly in the build script. IMHO the only other place would be to have it in TetGen.jl/deps/src. I don't think there should be an extra package for that.
    Let me know what you think.
  • So far I build the 64bit version. Should I build 32bit as well ?
  • I will add another small API which essentially maps all C++ arrays to Julia array like in Triangulate.jl.
  • I have the impression that the memory of the TetGen created arrays currently is not freed.
    I prepared for this point by slightly patching tetgen.h with new and delete operators maping onto malloc/free. This should allow to give ownership of the arrays to Julia (as in Triangulate). Not tested yet, though.
  • Once all of this works for 1.5, I will go to 1.6 (and try to establish a reasonable upstream location for it).
    Options I see are WIAS, github and zenodo.

@SimonDanisch
Copy link
Member

As for the c wrapper, I made a here script to create the file directly in the build script.

I think you can just have a file in the subfolder in yggdrasil via a DirectorySource, see: https://github.com/JuliaPackaging/Yggdrasil/tree/master/K/KaHyPar. This should be the way to go!

So far I build the 64bit version. Should I build 32bit as well ?

I'd just go for supported_platforms(), I don't think there was any issue with it.

Any memory management improvements would be great :)

@j-fu
Copy link
Member Author

j-fu commented Oct 14, 2020

Ok thanks.
as for 32/64 ... it was already late when I wrote that...
I meant Float64 and Float32. You have this kind of artificial templating in your folder and the option
to call TetGen with Float32 or Float64.

  • Should we both variants be supported ?
  • In C/C++ we have float and double which by the standard are not defined via their byte size. In that sense IMHO it is not consistent to call things "tetrahedralizeF64" etc. I assume that was also the reason to talk about Cdouble and Cfloat in Julia. IMHO naming should be consistent with that.
  • Certainly agree with DirectorySource... Will modify this for Triangulate as well when time permits.

@SimonDanisch
Copy link
Member

Should we both variants be supported ?

I think that would be valuable, although not highest priority. Would be nice, to at least convert in the julia wrapper to whatever is supported.

For double/single, I'd kind of hope we can rely on:

For x86 and x86_64 (amd64), you can rely on the types float and double being IEC-60559-conformant, though functions using them and operations on them might not be.

And using Cdouble doesn't seem to help with anything:
https://github.com/JuliaLang/julia/blob/c44644442949238da70670b547312ca1ae9a9c7d/base/ctypes.jl#L115

@j-fu
Copy link
Member Author

j-fu commented Oct 17, 2020

Ok, moved the C wrapper to DirectorySource.
Compiled now for all supported platforms and was able to test on linux / windows.
I also patched TetGen to use malloc/free, so this can be tried
For the float version, tetgen1.5.1. doesn't compile with -DREAL=float, I would postpone this
for 1.6.

So the plan is now:

At this stage, TetGenBuilder could be archived

  • Then: look into memory management, some additions to API: see https://github.com/j-fu/TetGen.jl/tree/j-fu/raw-api
  • EDIT: implement Julia interface for local refinement (tetunsuitable)
  • A bit of documentation
  • Once 1.6.1 is ready (Hang Si says soon, the float issue should be fixed by then) I
    would update build_tarballs to 1.6, with the additional handling of the float case

@j-fu
Copy link
Member Author

j-fu commented Oct 19, 2020

Hi, got the first steps with the "Triangulate-like" interface running;
j-fu@2448f37

We would have to agree about the naming of the different TetGenIO variants:

Now there is:

  • CPPTetGenIO
  • JLTetGenIO (was TetGenIO - I assumed it can be renamed as it was not exported)
  • RawTetGenIO

I very much would like to rename RawTetgenIO to TetGenIO. In any case I see this as an additional part
oft the API. Any higher level API on top of this should go into a different package.

@j-fu
Copy link
Member Author

j-fu commented Oct 21, 2020

Hi may be I was too brief and a bit robust in my communication, so let me explain:

What I meant by the additions to the API is now implemented in RawTetGenIO (besides of the local refinement stuff). RawTetGenIO essentially wraps the arrays returned by TetGen into Julia arrays. The API In Triangulate.jl is similar and works with a struct called TriangulateIO, without copying of data. Furthermore, in TetGen_jll.jl, TetGen has been patched so that new() and delete() are mapped on malloc() and free(), thus the Julia arrays now can get the ownership of the memory.

I would not touch the existing API which as far as I understand converts the data from CPPTetGenIO to different variants of Meshes, and I assume it will evolve along with GeometryBasics. Do you see the existing TetgenIO as part of the API ? If so I of course would revert the renaming.

I see various reasons for having an array based API along with the API based on the mesh structure:

  • it has almost zero overhead (as far as I comprehend the mesh based API, through convert() there is
    no difference here)
  • by keeping the items and names of the tetgen output, documentation is easy via relating to the TetGen documentation
  • essentially all TetGen functionality can be pushed through it. In the moment, the Voronoi stuff is missing, but this is used not very often and can be added later
  • lots of people coming from matlab etc. think in coordinate arrays and incidence matrices, so they can use this easily without learning other data structures
  • and yes, I come from the same camp but from C++ instead of matlab . Array based APIs provide lots of options for easy interacting with other packages. We implemented C++ PDE slovers based on 1-offset multidimensional arrays and had never problems to interact with other codes including Fortran based ones... And I do think that the success of matlab is very much based on the array-centeredness of the data structures there.

What I would like to avoid is a situation like in the case of Triangulate/Triangle where there are two packages.

@SimonDanisch
Copy link
Member

Why don't you add this as PR to discuss things there?

@j-fu
Copy link
Member Author

j-fu commented Dec 23, 2020

All of this is done, only waiting for TetGen 1.6.1 finalization by upstream. Closing this.

@j-fu j-fu closed this as completed Dec 23, 2020
@SimonDanisch
Copy link
Member

Great work, thank you :)

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

No branches or pull requests

2 participants