fixed meshing issues: issue #150 | discussion #156

[duarte-jfs]

Follow up from #150 (comment)

[duarte-jfs]

Follow up from #156 (comment)

As per the documentation on shapely for the split() function (https://shapely.readthedocs.io/en/stable/manual.html#shapely.ops.snap):

It may be convenient to snap the splitter with low tolerance to the geometry. For example in the case of splitting a line by a point, the point must be exactly on the line, for the line to be correctly split. When splitting a line by a polygon, the boundary of the polygon is used for the operation. When splitting a line by another line, a ValueError is raised if the two overlap at some segment.

[HelgeGehring]

Thanks, looks great!

My main worry would be: Is the mesh still exact after snapping? I'd expect a snap to change the mesh in case a line is passing close by a previously defined mesh point?

Putting there a fixed number in is a bit dangerous, e.g. I often use 1e-6m instead of 10um and snapping by 0.1 would then change everything... :/

Should we maybe export it as a parameter? Or set the snapping to a wayyy smaller number? (something like the sys.float_info.epsilon? Or would it then have no effect?

@simbilod Could you have a look?

[simbilod]

How about we use the MeshTracker's atol attribute to choose the snapping tolerance?

It is instanciated over at

femwell/femwell/mesh/mesh.py

Line 321 in d1f0919

meshtracker = MeshTracker(model=model)

we can set it as an argument to mesh_from_OrderedDict, and pass it when we construct there, and then whenever we call break_line_

[duarte-jfs]

That's a good idea! I'll try it and get back to you

[duarte-jfs]

@simbilod that solution has worked for both examples that I had previous problems with. I think it is fixed :)

[simbilod]

Nice! One more change below and we can merge :)

[simbilod]

In general I should start about deprecating this meshing API in favor of meshwell, but there are some features (like the named lines here) that meshwell does not support yet

[simbilod]

Suggested change

	def break_line_(line, other_line, snap_tol = 0.1):
	def break_line_(line, other_line, snap_tol):

Let's not assume a default value, we can let the program crash if this is called without a value

[simbilod]

one more thing I noticed after checking out the branch: I would avoid whitespace in directory and filenames. Could we use underscores?

[duarte-jfs]

Done :)

[simbilod]

Looks great, the example runs great for me!

The only remaining comment I have is that the packages pint and enlighten are not part of pyproject.toml, and so they need to be installed manually to run the example. This will cause an error when we build the docs online. @HelgeGehring should we add those dependencies under docs maybe?

[HelgeGehring]

Yeah, putting them under docs sounds good!

Pint doesn't need any further integration into femwell, right? Then it would be perfect to just have it in the docs to show that it works.

About enlighten: Up to now I used always tqdm. It's maintained a lot, but being able to print output while showing a progress bar sounds just great.

So probably best would be to just put the two packages and tqdm all in [docs] in the pyproject.toml (we'll then also need to update the docker file to also install those to keep the examples executable.)?
This way we allow the user to decide?

[HelgeGehring]

Ah and two extra things:

• please install pre-commit and run it (pip install pre-commit; pre-commit install; pre-commit run --all) that formats/... the code before commiting and keeps us from having changes because someone formatted something
• It would have been good to have two PRs in such a case, one for the meshing fix and one for the nice tutorial. Of course, the tutorial one can start from the first one. Just easier to review and I would have accepted the first one already :D but no worries we can just fix the last bit here and then I'll accept :)

[HelgeGehring]

(And you should add the email address you use for commiting to your github account, that way your commits get linked properly)

[duarte-jfs]

Thank you so much for the comments. I'm not experienced with git (and github) so if you feel anything is missing, please let me know.

As for the double emails, you're right, that was my bad. I did a push from the bash and another from vscode, each with different users. When I realized it was too late.

[HelgeGehring]

I'd guess the only thing missing is putting the dependencies in the pyproject.toml?

[simbilod]

Yes, and @duarte-jfs could also add themselves to the README as a contributor :)

[duarte-jfs]

Really sorry but just noticed I forgot to correct the name on the _toc.yml. I can create another pull request if it's easier for you

EDIT: nevermind, just saw the fix. Thank you

[HelgeGehring]

No problem :)

First of all, the page looks just amazing! I'll read it in detail and then provide more feedback :)
I also moved the references, like this they have links and everything.
Also I fixed the heading in the beginning, now the example has the right name :)

Some things I think we should work on:

• There's some code multiple times in the example, for example the mesh generation. We could either generate all meshes in the beginning or we could provide a function which returns a mesh and call it with different parameters. (Less code, easier to read, easier to maintain) there's I think also some other code more than once
• kb and T are not used at all, right? Should we remove them?
• For the other constants, we could just use the constants defined in scipy, we don't need to have the numeric values, right?
• We can probably generalize the calculations of I0,V0,R,L,C,G and put them in the mode class, right?
• I think the images from the paper are missing :/ maybe we should also just make our own sketches there?
• I didn't know where you used [3] and [4], so i just put them somewhere in the beginning

Should we maybe start a tracking issue where we discuss those things and split them in smaller PRs (as this one is already merged)

And again, nice notebook! I'll give it a more detailed read :)

[duarte-jfs]

Thank you so much!! I'm happy that is clear and hopefully it will help people down the line. Indeed, I think there is quite some room for improvement. I will open an issue for the tutorial and reply to those points so that we can keep track of it all.