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

Reverse ordering of corners of a triangular fault #3

Open
wants to merge 13 commits into
base: triangular_updates
Choose a base branch
from

Conversation

dsrim
Copy link

@dsrim dsrim commented Aug 2, 2018

The solver assumes that the corners (vertices) of the triangular subfault should be oriented counter-clockwise. If two vertices are swapped, the orientation reverses, negating the resulting deformation. This is now fixed when we check the sign of normal[2] (if the normal points downwards, it is oriented clock-wise), by reversing the ordering of given self._corners by self._corners.reverse()

@rjleveque
Copy link
Owner

Thanks for looking at this @dsrim. I thought it might be an assumption of orientation. A few comments.

  1. Now the two orientations give consistent-looking results but I think they are both wrong rather than both right. The uplift (red) should be on the up-dip edge. I also tried your notebook comninou_vs_okada.ipynb that cuts a rectangle into two triangles and the triangles give the negative of the rectangle. So probably it's in the opposite case that you want to reverse the order of vertices.

  2. My triangular_updates branch was already merged into master in WIP: Triangular Okada updates clawpack/geoclaw#321, so after fixing the orientation it would be better to do a PR to clawpack/master rather than to my branch.

  3. If possible it would be nice to apply Okada in a way that doesn't change the corners that the user sets up for the fault, e.g. by negating the displacement rather than by reversing the corners. It probably doesn't matter, but it is conceivable that the user is also doing other things with the fault geometry that requires a different orientation. Probably not within this DTopography object since we don't provide tools to do other things yet, but someone might want to extend it at some point. Don't bother if it's much harder to negate the slip for some reason, but in general it's nice to not mess with the data provided by the user when possible.

  4. I was wondering why you have rake as an argument to calculate_geometry_triangles, rather than just assuming that self.rake is set at this point? I understand it should be a parameter in set_corners since the user might call this to set up a new triangle, but then why not set self.rake in this routine rather than passing into calculate_geometry_triangles and doing it there? This seems dangerous since the user might call calculate_geometry_triangles without specifying a rake parameter because they already set self.rake and this routine would use the default 90. rather than self.rake.

  5. It would also probably be best to have no default value for rake in set_corners, to force the user to specify a rake since this is necessary. We initialize self.rake = None in the class SubFault for this reason. While 90 is a reasonable default for subduction zones, it is not always right and the user should have to think about it.

  6. I notice pyproj and utm are new dependencies that users will have to install to use this. We should comment on this somewhere. But I also wonder if we should be using projections like this when applying Okada on rectangles too? Presumably this is why there are slight differences between Okada on a rectangle and the new routines on two triangles from the rectangle?

… set ``self._fix_orientation`` flag then fix the sign

- removed ``rake=90.`` as an optional input argument to ``self.calculate_geometry_triangles()`` and made ``rake`` required for ``self.set_corners()``
rjleveque pushed a commit that referenced this pull request Sep 21, 2018
Triangular angular - fix rise_time functions
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.

3 participants