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

Shapely preprocessor #66

Closed
wants to merge 70 commits into from
Closed

Shapely preprocessor #66

wants to merge 70 commits into from

Conversation

connorferster
Copy link
Collaborator

Here is the pull request for review of the shapely pre-processor. This introduces several breaking changes to the pre-processor and is intended to commence as v2.0.0 of sectionproperties.

connorferster and others added 30 commits December 19, 2020 21:02
…nstantiation; post.py: altered plot finalization func to prevent duplicate legend entries; sections.py: complete implementation of Geometry and new CompoundGeometry classes, doc strings to follow
…rations to init(), fixed typos, altered signature to CrossSection
…multiple files; Updated: alignment methods in sections.py
…o be causing test runs to prematurely terminate with no tests running.
…ry objects, along with signatures and doc strings. Add possiblity of creating Geometry instances with just ordered points, not just Polygons <in progress>
…ms with some values); Fix: update create_points_and_facets to not append the first point in the geometry twice; Fix: change plot title strings with backslashes to raw strings
…ments was not defined. Tests: Updated test_validation.py and test_rectangle.py to run with pytest-check. Fix: Bug in sections.py where an extra facet would be created for holes.
…gging; Fix: updated PlasticSection to make it compatible with shapely preprocessor (still in progress); Tests: updated tests in test_rectangle.py
….py; search for 'log.log' to find all instances. Some of have been commented out.
…e main branch and that all tests now pass. Increased test coverage to all warping properties.
…operations. Passes tests but not new composite test.
…egan updating Geometry with split_section() method. Created bisect_section.py

geometry_1 = sections.i_section(d=203, b=133, t_f=7.8, t_w=5.8, r=8.9, n_r=8)
geometry_2 = sections.rectangular_section(d=20, b=133)
compound = geometry_2.align_center(geometry_1).align_top(geometry_1) + geometry_1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@connorferster, update align_top as no longer exists in code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And that one!

return (top_geoms_acc, bottom_geoms_acc)


def offset_section_perimeter(self, amount: float = 0, resolution: float = 12):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@connorferster, should this also be renamed to offset_perimeter like in Geometry class to be consistent, as every other geometry manipulation is named consistently between the two geometry classes??

(and all example updated if changed?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! Updated and fixed.

points: List[List[float]],
facets: Optional[List[List[int]]] = None,
holes: Optional[List[List[float]]] = None,
control_points: Optional[List[List[float]]] = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@connorferster, curious if there is ever a use case envisaged where you would want to specify by hand a control_point vs automatically generating it to be within the geometry/section. Potentially seems a little redundant, and one of the benefits to going to shapely was to have them autogenerated vs having to think about specifying them manually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Agent6-6-6, I ran into a use case when trying to create a concrete composite column: square concrete column with steel I-section in the middle. Both shapes auto-generated a control point in the center of their respective shapes, creating an ambiguous overlapping region.

This brings up the other point you brought up, specified control points need to persist with the Geometry as it is transformed. I am going to add a flag to Geometry to deal with this. Will be coming in the next commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will create a test for it also.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see.

See one of my last replies in #21 as I think there is a way to automatically create the control points within each bit of geometry. In the case of your composite column case, recognising that the concrete part of the section actually contains a hole in reality where the steel is, you can split up/process the section into the constituent bits without any overlaps and then assign the control points automatically.

If you could get something like that working it almost means the control_point concept is just an internal requirement that is worked out to aid in the meshing process. Then the user doesn't need to worry about it at all (ideal world) .... I'm sure I'm over simplifying things though!!

… Flit; Bugs: Fix faulty behaviour in Geometry.align_to and update docstring
…iors and exteriors independently. Tests: Fixed broken test for CompoundGeometry.from_points()
@connorferster
Copy link
Collaborator Author

connorferster commented Jun 7, 2021

Hi @Agent6-6-6,

I have updated the code to include the ability to assign a control point. For now I think having assignable control point that "travels" with the section allows for the greatest flexibility. I hear what you are saying in regards to a composite column (with the steel actually filling a void within a concrete column). However, that will be up to the user to decide as they build the geometry: I don't think we could come up with a rule that would work in all situations without making things complicated. The new preprocessor allows for the creation of invalid or ambiguous geometry which the designer is required to make unambiguous.

So the user would have to do something like:

(conc_section.assign_control_point([x,y]) - i_section) + i_section

As far as this PR goes, I believe it is largely complete (barring any other bug fixes, of course!). I am focusing now on continuing to update all of the documentation to reflect all the new features. I am also planning a new section called "Advanced Geometry Manipulation" to describe some of the issues and techniques that we have been talking about. Things like how to use shape operations to create intermediate nodes.

"""Executes code required to finish a matplotlib figure.

:param ax: Axes object on which to plot
:type ax: :class:`matplotlib.axes.Axes`
:param bool pause: If set to true, the figure pauses the script until the window is closed. If
set to false, the script continues immediately after the window is rendered.
:param string title: Plot title
:param str title: Plot title
Copy link
Contributor

@Czarified Czarified Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You added size and dpi into the default arguments. Should they be added to the docstring as well? It's obvious for anyone who's worked with matplotlib, but the same could be said for the title arg...

:param int size: Plot size
:param int dpi: Plot dpi

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% Agree. Thanks for the catch!

if not dxf_filepath.exists():
raise ValueError(f"The filepath does not exist: {dxf_filepath}")

# TODO avoid step of making a temp file locally
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like we're writing a temp file now. Does this function actually save a temp file, or is this comment outdated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was not sure what's going on there. It does not seem like we are writing a temp file so I can remove that.

Copy link
Contributor

@Czarified Czarified left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't reviewed the examples, or tests, and I only skimmed the sections.py, which is actually the biggest change here. I just don't have enough time right now to really dig in. From the past months where I've played with examples and read your comments/thoughts, I think the UX with this change will be significantly improved.

Requiring cad_2_shapely for dxf makes sense, but for whatever reason it doesn't sit well with me. Should we include it as an actual requirement, instead of a custom error message? It's a rather small library by our friend @aegis1980, so it may just make sense to include as part of sectionproperties >2.1, if they desire to merge it in.

I'll try to review the rest of it this week, and play around with some examples as well. No promises. 😄 I love all the changes you've made, and really want to see v2.0.0 this year!

@connorferster
Copy link
Collaborator Author

connorferster commented Jun 8, 2021

@Czarified I agree with your thoughts on cad_2_shapely. I think it would be good to include it as an actual dependency and a full-fledged feature of sectionproperties instead of as a kind of optional add-on. @aegis1980: do you have any thoughts/feelings/preferences on either making your library a dependency of sectionproperties or having sectionproperties vendor it in directly? It works wonderfully and is a great feature to have as part of sectionproperties. Are you planning on developing it further and maintaining it? If so, then I think it would be good to keep as a dependency so sectionproperties can benefit from future development.

@Agent6-6-6
Copy link
Contributor

Hi @Agent6-6-6,

I have updated the code to include the ability to assign a control point. For now I think having assignable control point that "travels" with the section allows for the greatest flexibility. I hear what you are saying in regards to a composite column (with the steel actually filling a void within a concrete column). However, that will be up to the user to decide as they build the geometry: I don't think we could come up with a rule that would work in all situations without making things complicated. The new preprocessor allows for the creation of invalid or ambiguous geometry which the designer is required to make unambiguous.

So the user would have to do something like:

(conc_section.assign_control_point([x,y]) - i_section) + i_section

As far as this PR goes, I believe it is largely complete (barring any other bug fixes, of course!). I am focusing now on continuing to update all of the documentation to reflect all the new features. I am also planning a new section called "Advanced Geometry Manipulation" to describe some of the issues and techniques that we have been talking about. Things like how to use shape operations to create intermediate nodes.

Yeah I get what you're saying, perhaps a feature for the future. I still believe there is a way to achieve recognising each overlapping geometry and adding control points intdrnally/automatically, I just haven't had much luck with getting it to work as intended, apart from with two geometries as I indicated previously. I need to give it a bit more of a serious effort sometime in the future.

I wonder in the interim if something to report back to the user regarding the meshing not working might consist of reporting the max mesh size actually formed during meshing, vs the defined mesh size. This would highlight that the user that the defined mesh of say 5mm^2 max size wasn't achieved by reporting back that it was 25mm^2 for example. This may indirectly indicating a potential meshing issue involving control points not working due to bits of resulting overlapping section not having or ending up with a control point within them? This might prompt the user to review the geometry/mesh plots if they were not already and manually add further control points as required to mesh things correctly.

I'd just prefer to see something warning user one way or the other that the mesh went a bit off the rails. At the moment it relies on the user recognising this by reviewing plots which is a bit archaic. To streamline things after initially setting up a script one might disable the plotting for example.

…ference to temporary file and made cad_to_shapely a project requirement
@aegis1980
Copy link

aegis1980 commented Jun 20, 2021

Hey all,
Sorry. I turned off notification etc for section-properties, but just got a notification about cad-to-shapely and found this discussion.
I have no plans for working on cad-to-shapely really. Will just let it (and repo) sit there. Was actually developed to aid another unrelated project and I thought I could use it with section-properties (and ran into the meshing issues and gave up).
My inclination would be to keep it as an independent dependency since perhaps Mr/Mrs SomeOne might find a use for cad_to_shapely and develop which could then benefit section-properties and whatever else.
@connorferster As an aside, did you try running the example dxfs (tricky aluminium extrusions) in cad-to-shapely repo through updated section-properties?
https://github.com/aegis1980/cad-to-shapely/tree/master/example_files

@connorferster
Copy link
Collaborator Author

Closing PR to merge instead onto v2 branch

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

Successfully merging this pull request may close these issues.

5 participants