-
Notifications
You must be signed in to change notification settings - Fork 69
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
Convert magnetic vector to inclination and declination #402
Conversation
It returns the intensity, inclination and declination from a magnetic vector
It returns the 3-component of the magnetic vector from the intensity, inclination and declination
The other version has a problem in the condition to calculate the inclination when the horizontal-component is zero. I fix it using a NumPy mask
`magnetic_ang_to_vec`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great @aguspesce! I made some suggestions for changes, let me know what do you think.
I think the tests might need some work. For example, we are not testing against degrees=False
and the pytest fixtures could be a little bit more readable.
Besides, for some reason tests aren't catching the bug in the last line of magnetic_vec_to_ang
(where we return only the first element of inclination
and declination
). This is a sign that tests should be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome! Just some minor suggestions for the docstring.
Also, should we make a note somewhere, that our coordinate is upward up, so we have a negative magnetic vector (magnetic_u) with downward magnetization? Sometimes the upward is down, which might be a little confusing for people, haha.
harmonica/_utils.py
Outdated
|
||
def magnetic_ang_to_vec(intensity, inclination, declination): | ||
""" | ||
Convert intensity, inclination and declination angles of the magnetic field to a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call it magnetic field or magnetization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions could be use with either magnetization vectors or magnetic vectors. We could improve the docstrings to make it more general: magnetic field or magnetization vector
could be better suited here.
Thanks for the review @LL-Geo! You're right that we should probably be more explicit about directions. For Harmonica I'm considering that both for the magnetic and magnetization vectors, the vertical component points upwards. That's why we are using
Sounds like an Escher's painting haha. Sometimes people have coordinate systems with vertical pointing down. We try to overcome that confusion by explicitly putting the word upward. The tricky thing in these functions is that the inclination angle is defined as positive when the vector points downwards. So, the equations for transforming them are not exactly the ones in Blakely (1995): in there they use z->down. |
I like that painting! hahaha. That's the world in my dream🤣 Yeh... 'Traditional' coordinates usually point vertically down. And always confuse people. One day I suddenly realised that the positive density generates negative gravity in SimPEG due to the definition of "positive up and down". hahaha... |
Indeed! That's why we compute the |
Improve docstring and the function code Co-authored-by: Lu Li <[email protected]> Co-authored-by: Santiago Soler <[email protected]>
Use the NUMBER option for docstests so pytest compares floats up to the expected precision.
Replace for loops for parameterizations. Add a second test for the identity.
…onica into mag-ang-vec-transformation
@aguspesce I think this is ready to be merged after I took the liberty to make a few changes. Would you like to review it and share your thoughts? Although it would be nice to have some example for these two functions, I think we can avoid including them here and just use these functions in other example that forward models some magnetic field. For example, the ones in #369 Thanks a lot for the time you put into this! |
@santisoler and @LL-Geo thanks you for your collaboration on this and finished this code for me. |
Create functions to convert the three components of magnetic vectors to the intensity, inclination a declination, and vice-versa. Add new functions to the API reference and add tests for them.