Improvements to Moulder #411
Conversation
There was a problem hiding this comment.
@santis19 fantastic work!
This just needs some tests for the polygon area/orientation and the missing docstrings. I left some inline comments.
If you can, could you please submit the Polygon changes in a separate PR? That will help clean up the project history. No need to do anything fancy with git. Copy your new mesher/geometry.py to a new branch and then copy the master mesher/geometry.py to this one (and commit). Merging the PR will create a single commit anyway.
| @@ -72,9 +72,11 @@ class Polygon(GeometricElement): | |||
|
|
|||
| """ | |||
|
|
|||
| def __init__(self, vertices, props=None): | |||
| def __init__(self, vertices, props=None, force_clockwise=False): | |||
There was a problem hiding this comment.
It's probably better to default this to True since having counterclockwise vertices is usually a bug.
| if area < 0: | ||
| return "CW" | ||
| else: | ||
| return "CCW" |
There was a problem hiding this comment.
Please change CW and CCW to clockwise and counterclockwise for readability.
| return "CCW" | ||
|
|
||
| def set_clockwise(self): | ||
| self.set_orientation("CW") |
There was a problem hiding this comment.
This method is kind of unnecessary. Just having set_orientation is probably enough since this doesn't really save a lot of typing.
| raise ValueError("Orientation must be 'CW' or 'CCW'") | ||
| current = self.orientation | ||
| if orientation != current: | ||
| self._vertices = self._vertices[::-1] |
There was a problem hiding this comment.
A more Pythonic way of implementing this method would be as a property setter so that we could do polygon.orientation = 'clockwise' to set it:
@orientation.setter
def orientation(self, new_orientation):
The rest stays the same.
| If False, the area can be either positive or negative. | ||
| The sign gives information about the orientation of the vertices: | ||
| If area is negative, the vertices are orientend clockwise. | ||
| if area is positive, the vertices are oriented counter-clockwise. |
There was a problem hiding this comment.
Please include the citation that you mention in the PR description.
| if orientation != current: | ||
| self._vertices = self._vertices[::-1] | ||
|
|
||
| def get_area(self, absolute=True): |
There was a problem hiding this comment.
This should probably be a private method because the absolute parameter doesn't make a lot of sense from a user perspective (but we need it for orientation). I would suggest turning this into _compute_area without the absolute value. Then we can use this to calculate the orientation. And having an area property that calls _compute_area and applies the absolute value always.
|
@santis19 thanks for the extra PR. We should probably merge these changes to Moulder here first and then port it to the new repo once that gets started. If you want to copy over some of the stuff from https://github.com/fatiando/geometric to the new repository, that would help. Don't worry about setting up the docs right now. |
With this PR I intent to improve the gravmag.interactive.Moulder class (the interactive app to create 2D gravity models with Polygons).
I have added to Moulder:
sharex=Trueas default in figure creation in order to associate the model ax with the result ax.And I have edited the mesher.Polygon class, adding a method to compute the area using the Shoelace Formula.
I have also added an option to avoid the application of the absolute value, so the area can be either positive or negative.
If the area is positive, the vertices of the Polygon are ordered counter-clockwise, while if it is negative, they are oriented clockwise.
This allowed me to create methods inside Polygon to change the orientation.
Using these new methods, Moulder forces the Polygons to be always clockwise in order to avoid mistakes on how the gravity field is computed (previously, if the Polygon was define counter-clockwise the gravity field was the one generated by the same Polygon with the opposite density).
Checklist:
doc/install.rst,environment.yml,ci/requirements.txt.cd doc; makelocally)doc/contributors.rst(leave for last to avoid conflicts)