Split mesh creation function

[gnath-aswin]

Organize src/meshpy/mesh_creation_functions Folder

This PR addresses issue #319 of organizing the src/meshpy/mesh_creation_functions folder.

Changes Made

1. Moved all application-style beam mesh modules to mesh_creation_functions/applications:

   • beam_stent.py
   • beam_honeycomb.py
   • beam_fibers_in_rectangle.py
   • beam_wire.py

2. Split basic functionalities from beam_basic_geometry.py into new modules:

   • beam_line.py:
     Contains create_beam_mesh_line and create_beam_mesh_line_continuation
   • beam_arc.py:
     Contains create_beam_mesh_arc_segment_via_rotation, create_beam_mesh_arc_segment_via_axis,
     create_beam_mesh_arc_segment_2d, and create_beam_mesh_arc_at_node
   • beam_helix.py: contains create_beam_mesh_helix

3. Left beam_basic_geometry.py with a deprecation warning:

   • Exposes the new functions using internal aliases

4. Renamed modules for clarity:

   • beam_curve.py → beam_parametric_curve.py
   • beam_nurbs.py → beam_from_nurbs_curve.py

[davidrudlstorfer]

@gnath-aswin thanks for your first contribution!

This PR is already looking really good. I have a few points for discussion:

• I think we can remove the deprecation stage and just switch to the new layout. MeshPy only has a very small number of active users/developers and, therefore, we can simply switch.

• Should we directly update the docstrings in the touched files to align with our new style?

• Should we directly split up the tests to reflect the src code files?

[isteinbrecher]

Thanks for the contribution @gnath-aswin ! I left some comments to apply / discuss.

I agree with the points of @davidrudlstorfer (splitting test files, adapting the doc string style), but I would do that in separate PRs.

[isteinbrecher]

I really like this approach of deprecating old functionality, but I agree with @davidrudlstorfer that this might be an overkill in this case. I would just remove this file.

[isteinbrecher]

@gnath-aswin general note: In GitHub we use threads like this to discuss pats of the code. When the initial issue is solved / an agreement is reached, you can close this conversation (with the button Resolve conversation). This allows to easily keep track which discussions are still open. Also, all discussions need to be resolved so that we can merge. If you are not sure if the discussion can be resolved just ask the people who opened it.

For example this discussion can be resolved, since it was about the deprecation file - we agreed to remove it, and you did, so we are done here!

[isteinbrecher]

Not related to this file, but I would rename these two functions like:

• create_beam_mesh_function -> create_beam_mesh_generic
• create_beam_mesh_curve -> create_beam_mesh_parametric_curve

[isteinbrecher]

With this (and create_beam_mesh_arc_at_node) I am a little bit torn, I think I would prefer to have them in a single file beam_node_continuation.py, since they are usually always used together. Is there a specific reason you put them in the respective line and arc files?

[gnath-aswin]

My initial thought was to group them with their respective geometry types (line or arc). I agree that on usage perspective, it would actually make more sense to have both in beam_node_continuation.py .

[isteinbrecher]

General question (@davidrudlstorfer @knarfnitram), since this would be a good point to do so, are we happy with the naming:

create_beam_mesh_xxx(...)

The initial thought behind this was that this naming should indicate that a beam related mesh is created and not a geometry, i.e., that the discretisation is performed within these functions. They names can become lengthy, but I actually don't mind that and think that for such relevant functions a descriptive name is important - but I am happy to hear your thoughts.

[davidrudlstorfer]

General question (@davidrudlstorfer @knarfnitram), since this would be a good point to do so, are we happy with the naming:

create_beam_mesh_xxx(...)
The initial thought behind this was that this naming should indicate that a beam related mesh is created and not a geometry, i.e., that the discretisation is performed within these functions. They names can become lengthy, but I actually don't mind that and think that for such relevant functions a descriptive name is important - but I am happy to hear your thoughts.

Thanks for your review @isteinbrecher. In my opinion we could drop the mesh because I think that this should be self explanatory. But I have no strong opinion on that so we can also keep it as is. All solutions are fine for me

[knarfnitram]

One the one hand mesh is redundant so in terms of good practice, we could get rid of it and have a simpler interface, on the other hand by using auto completion within an proper IDE it should be easily completed in any case.
So I have also no strong opinion on that and can also live with both.

[gnath-aswin]

Thanks for the feedback @davidrudlstorfer @isteinbrecher @knarfnitram . Below is a summary of the updates I will include in this PR, along with suggestions for tasks that can be handled in a separate PR.

Changes to be made in this PR

• Remove beam_basic_geometry.py completely instead of deprecating it.
• Rename the following functions for clarity:
  • create_beam_mesh_function → create_beam_mesh_generic
  • create_beam_mesh_curve → create_beam_mesh_parametric_curve
• Move node-related functions to a new module beam_node_continuation.py:
  • Move create_beam_mesh_line_at_node from beam_line.py
  • Move create_beam_mesh_arc_at_node from beam_arc.py

Suggested tasks for a follow-up PR

• Update docstrings to new style.
• Split the test file test_mesh_creation_functions.py based on the new module structure.

Naming Convention Discussion

Since there's no strong consensus to change it right now, My suggestion would be we keep the current naming pattern for now (create_beam_mesh_xxx) and revisit this if needed in a future refactor.

Let me know if you have any more suggestions.

[isteinbrecher]

Let's remove this part of the comment as it does not really give any useful information and is prone to be outdated soon.

[isteinbrecher]

@gnath-aswin thanks for posting the changes to this PR. There is no exact rule on how to go about this, but small points like the one in this PR are fine if you just resolve within the discussions, this allows to better structure the PR. As always it depends on the actual case and there are no strict rules. I like your proposed approach, feel free to add the changes and push to this branch.

One additional note: We don't have a rule on how commit messages should look like in MeshPy, but they should be somewhat meaningful. In your example you have two commits, both with the message added specific geometries to applications and split beam_basic_geometry to beam_line, beam_arc and beam_helix. The first one was your initial draft with some unwanted changes and the second one was the current state. In this case, I would squash them together into a single commit with a message like "Split up mesh creation functions". It is ok to have multiple commits, also ones like "Apply PR comments" (of course always depending on the current case). I suggest you have a look at git rebase ... and git rebase -i... (https://git-scm.com/book/en/v2/Git-Branching-Rebasing). We can also have a look at this together at some point next week.

I know this is a lot to handle at the beginning, but trust me you will get used to this very quickly!

[isteinbrecher]

@gnath-aswin is there any update on this PR?

[gnath-aswin]

@gnath-aswin is there any update on this PR?

changes have been made to the branch, please review @isteinbrecher

[isteinbrecher]

@gnath-aswin thanks for the changes. There is a merge conflict with the current main branch. This has to be resolved before this can be merged. This can either be done by merging main into your branch, or by rebasing your branch on the current main. In this case I would prefer a rebase - if you do this in VisualStudioCode then it should be hopefully straight forward. You can have a look at this tutorial (https://medium.com/@slamflipstrom/a-beginners-guide-to-squashing-commits-with-git-rebase-8185cf6e62ec) and we can discuss if there are any questions.