Closed duarte-jfs closed 3 months ago
Follow up from https://github.com/HelgeGehring/femwell/discussions/156#discussion-6617560
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.
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?
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
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?
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?
Ah and two extra things:
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(And you should add the email address you use for commiting to your github account, that way your commits get linked properly)
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.
I'd guess the only thing missing is putting the dependencies in the pyproject.toml
?
Yes, and @duarte-jfs could also add themselves to the README as a contributor :)
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
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:
kb
and T
are not used at all, right? Should we remove them?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 :)
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.
Follow up from https://github.com/HelgeGehring/femwell/issues/150#issue-2253377705