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

Convert magnetic vector to inclination and declination #402

Merged
merged 28 commits into from
Oct 6, 2023

Conversation

aguspesce
Copy link
Member

@aguspesce aguspesce commented Apr 22, 2023

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.

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
@santisoler santisoler changed the title WIP - Create a function to obtain the msgnetic vector components from inclination adn declination angle WIP - Convert magnetic vector components to inclination and declination angles Apr 24, 2023
@santisoler santisoler changed the title WIP - Convert magnetic vector components to inclination and declination angles WIP - Convert magnetic vector components to inclination and declination Apr 24, 2023
@santisoler santisoler changed the title WIP - Convert magnetic vector components to inclination and declination WIP - Convert magnetic vector to inclination and declination Apr 24, 2023
Copy link
Member

@santisoler santisoler left a 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.

harmonica/_utils.py Show resolved Hide resolved
harmonica/_utils.py Outdated Show resolved Hide resolved
harmonica/_utils.py Outdated Show resolved Hide resolved
harmonica/_utils.py Outdated Show resolved Hide resolved
harmonica/_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@LL-Geo LL-Geo left a 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 Show resolved Hide resolved
harmonica/_utils.py Outdated Show resolved Hide resolved
harmonica/_utils.py Outdated Show resolved Hide resolved

def magnetic_ang_to_vec(intensity, inclination, declination):
"""
Convert intensity, inclination and declination angles of the magnetic field to a
Copy link
Member

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?

Copy link
Member

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.

@santisoler
Copy link
Member

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.

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 magnetic_u and not magnetic_z.

Sometimes the upward is down, which might be a little confusing for people, haha.

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.

@LL-Geo
Copy link
Member

LL-Geo commented Apr 28, 2023

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...

@santisoler
Copy link
Member

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 dreamrofl

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 g_z in Harmonica, but explicitly saying that it's the downward component, although our coordinate system has the vertical pointing upwards.

@santisoler santisoler changed the title WIP - Convert magnetic vector to inclination and declination Convert magnetic vector to inclination and declination Aug 16, 2023
@santisoler
Copy link
Member

@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 santisoler added this to the v0.7.0 milestone Sep 8, 2023
@aguspesce
Copy link
Member Author

@santisoler and @LL-Geo thanks you for your collaboration on this and finished this code for me.

@santisoler santisoler merged commit e885239 into main Oct 6, 2023
20 checks passed
@santisoler santisoler deleted the mag-ang-vec-transformation branch October 6, 2023 23:34
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