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.

Sign up for GitHub

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

Jump to bottom

plate_motion refactoring: remove pkg dependency #868

Merged
merged 17 commits into from
Oct 30, 2022
Merged

plate_motion refactoring: remove pkg dependency #868

merged 17 commits into from
Oct 30, 2022

Conversation

yuankailiu
Copy link
Contributor

@yuankailiu yuankailiu commented Oct 13, 2022 •
edited by yunjunz
Loading

Description of proposed changes

  • Use matrix rotation of the cartesian frame to compute the plate motion from the Euler pole

  • Add reference to Pichon et al., 1973, Cox et al., 1986, ESA webpage

  • thus we can remove the dependency on (1) astropy, and (2) platemotion: https://github.com/lcx366/PlateTectonic

Reminders

  • Pass Codacy code review (green)
  • Pass Circle CI test (green)
  • Make sure that your code follows our style. Use the other functions/files as a basis.
  • If modifying functionality, describe changes to function behavior and arguments in a comment below the function declaration.
  • If adding new functionality, add a detailed description to the documentation and/or an example.

@yunjunz yunjunz self-requested a review October 13, 2022 02:31
@yunjunz
Copy link
Member

yunjunz commented Oct 15, 2022 •
edited
Loading

Thank you @yuankailiu for this PR, it's very exciting! Removing the extra dependencies is critical for us to integrate it into the routine workflow.

After looking at it, it makes a lot of sense to create an object/class for the Euler pole initiation/calculation, maybe in an independent script, e.g. objects/euler_pole.py, in a similar form as https://github.com/joferkington/euler_pole/blob/master/euler_pole/euler_pole.py. We could try it in the coming week.

It's also a good time to include a basic unit test (#402) to compare the calculated PMM at a few selected lat/lon using mintpy with the one using the unavco web calculator (https://www.unavco.org/software/geodetic-utilities/plate-motion-calculator/plate-motion-calculator.html). After the above is finished I assume.

@yunjunz yunjunz changed the title plate_motion.py: remove pkg dependency plate_motion refactoring: remove pkg dependency Oct 15, 2022
yuankailiu and others added 8 commits October 28, 2022 12:14
@yuankailiu
plate_motion.py: remove pkg dependency
7dcd655 
+ Use matrix rotation of the cartesian frame to compute the plate motion

+ Add reference to Pichon et al., 1973, Cox et al., 1986, ESA webpage

+ thus we can remove the dependency on:
    1) astropy
    2) platemotion: https://github.com/lcx366/PlateTectonic
@yunjunz @yuankailiu
isort module import
c5702bb
@yunjunz @yuankailiu
simplify the setup/install for plate_motion
1185663
@yunjunz @yuankailiu
light refactor - part 1
f791f5e 
+ merge correct_plate_motion() into run_plate_motion() as the former is too simple

+ use mintpy docs/comments style for most of the functions

+ rename and merge T_*() into rotation_matrix_cart2enu() to be more explicit and reduce redundancy

+ remove the unused azimuth()

+ turn ON ellps by default
@yuankailiu
create mintpy/objects/euler.py
4421ef1 
+ create an object of the  EulerPole class for Euler pole initiation/calculation
  (a similar form as https://github.com/joferkington/euler_pole/blob/master/euler_pole/euler_pole.py.)

+ plate_motion.py calla the class and functions from objects/euler.py
@yuankailiu
create tests/objects/euler.py
a10ee27 
+ include a basic euler.py unit test to compare the calculated PMM at a few selected lat/lon

+ comparing using mintpy with the one using the unavco web calculator
  (https://www.unavco.org/software/geodetic-utilities/plate-motion-calculator/plate-motion-calculator.html).
@yuankailiu
formating via pre-commit
e148492
@yuankailiu
objects/euler_pole: refactoring
882090b 
+ rename class functions and variables as MintPy style
+ user readable variable names in class functions
+ rename the object module name to `euler_pole.py`
+ add assertion in tests/objects/euler_pole.py
+ remove pandas dependency
+ codacy tests
@yuankailiu
Copy link
Contributor Author

Hi @yunjunz, I am done with the changes. Please take a look!

@yunjunz
Copy link
Member

yunjunz commented Oct 28, 2022

Hi @yunjunz, I am done with the changes. Please take a look!

Great, I will check it now. Thank you @yuankailiu for the refactoring.

yunjunz and others added 5 commits October 28, 2022 17:27
@yunjunz
euler_pole refactoring v2
e8df949 
+ use snake_case for function names
+ get_velocity_xyz(): support 1/2D np array inputs
+ move get_azimuth_angle_from_east_north_obs() to utils.utils0.py
+ add tests/objects/euler_pole.py to the circle CI test
+ adjust other scripts accordingly
@yuankailiu
typo of rotationRate
3cf5676 
+ change rotationRate to rot_rate
@yunjunz
use pole_lat/lon for more explicit description
d1512ab
@yunjunz
EulerPole: add name and __repr__()
736566f
@yunjunz
tests/euler_pole refactoring
14915b1
@yunjunz
Copy link
Member

yunjunz commented Oct 30, 2022

Hi @yuankailiu, one last question on this PR: the calculated plate motion in the vertical direction is forced to zero in the previous version (

MintPy/mintpy/plate_motion.py

Line 133 in c15ad0f

vu = np.zeros(lats.shape, dtype=np.float32)
), but this is not the case for this PR. Shall we still enforce it?

@yuankailiu
Copy link
Contributor Author

Hi @yuankailiu, one last question on this PR: the calculated plate motion in the vertical direction is forced to zero in the previous version (

MintPy/mintpy/plate_motion.py

Line 133 in c15ad0f

vu = np.zeros(lats.shape, dtype=np.float32)

), but this is not the case for this PR. Shall we still enforce it?

I think we can keep the verticles. People will need to understand when using the non-spherical Earth, we will inevitably have some tiny verticles.

@yunjunz
Copy link
Member

yunjunz commented Oct 30, 2022 •
edited
Loading

I think we can keep the verticles. People will need to understand when using the non-spherical Earth, we will inevitably have some tiny verticles.

Thank you Kai. Good point.

Related to the vertical component, when ellips=False, the resulting vertical component is extremely tiny, which is expected, but the spatial pattern is strange (due to the numerical precision of the computer I guess), as shown below. Shall we set that to zero in euler_pole.py in that scenario, to avoid potentially unnecessary questions from users?

  • San Francisco Sen DSC - Assume Ellipsoid

Ellips

  • San Francisco Sen DSC - Assume Sphere

Sphere

@yuankailiu
Copy link
Contributor Author

I see. Thanks a lot for testing this! Yes, I think in that case (when ellips=False), we can just set it to zero. Good point.

yunjunz
yunjunz approved these changes Oct 30, 2022
View reviewed changes
Copy link
Member

@yunjunz yunjunz left a comment

Choose a reason for hiding this comment

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

Thank you @yuankailiu for this PR! I really like its structure: the class definition and unit tests could serve as a template/example for future new features.

@yunjunz yunjunz merged commit 930cbf4 into insarlab:main Oct 30, 2022
@yuankailiu yuankailiu deleted the dev.pm branch October 31, 2022 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yunjunz yunjunz yunjunz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@yuankailiu @yunjunz