-
Notifications
You must be signed in to change notification settings - Fork 22
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
Re-normalize the frequency-domain response in case of a magnetic source #45
Conversation
…ce prior to transform to time-domain
Thanks Ralph-Uwe for your contribution! Improvements to the magnetic-side of things are very welcome, as I hardly ever use that part of the code. Couple of questions:
Two tests fail currently. The first one is a regression test, so expected to fail. The second one, however, should not fail. I will have to look into that in more detail. I'll leave some comments in the code too. If you could change that great. Or, if you don't mind, then I'll do it when I get around to adjust the tests (probably after my holidays, so give me two weeks....) |
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.
Thanks for the PR! As I already mentioned, if you know of an analytical solution that we could use for comparison that would be great!
empymod/model.py
Outdated
# back into time-domain has to be carried out. | ||
for kk in range(len(freq)): | ||
EM[kk, :] *= 2j * np.pi * freq[kk] * np.pi * 4e-7 | ||
|
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.
Would you mind changing
for kk in range(len(freq)):
EM[kk, :] *= 2j * np.pi * freq[kk] * np.pi * 4e-7
to the more pythonic
for kk, kfreq in enumerate(freq):
EM[kk, :] *= 2j * np.pi * kfreq * np.pi * 4e-7
here and in the other case too?
Thinking about it I think we should actually avoid the loop entirely:
EM *= 2j * np.pi * freq[:, None] * np.pi * 4e-7
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.
(Relevant codacy feedback: https://app.codacy.com/manual/prisae/empymod/pullRequest?prid=4165816)
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.
That's perfectly ok!
A simple function for checking the impulse response due to a vertical magnetic dipole is: def dhzdt(t, sigma, r):
################################################# |
Fantastic, thanks Ralph-Uwe! Do you have a reference for the analytical solution? I might get time to include it tomorrow and add the relevant tests, but more likely it will have to wait two weeks until I am back. Either way, I will make a new release with the fix pushing it to Thanks again for the fix. And a big hooray, you are actually the first contributor (as far as code goes) to empymod! I will include you in the CREDITS file, and probably change the (c) from my name to something more generic like "(c) The empymod developers". I hope you found empymod useful (despite the bug ;-) ). |
Thanks Dieter, Kudos for the empymod package! Here's the reference: @incollection{ward1988electromagnetic, ...have a nice holiday! |
OK. I thought about it again, and talked it through with Evert Slob, who was one of the co-others of the paper on which the code is based (Hunziker et al., 2015, Geophysics). So currently it returns the H-field, the field due to a magnetic dipole. Your PR would turn that into a B-field for a coil, which turns your PR in a feature request rather than a bug fix. So it would have to be implemented optionally, not exclusively. And also, if we implement it for source coils we should implement it for receiver coils too. I'll have a think about it. Maybe an additional input parameter for Inputs/ideas/opinions welcome! |
One possibility is to have for Do you mind if I work directly on your PR, @ruboerner ? |
I think it might be better to introduce a new function for magnetic loops as "bipole" suggests that electric bipoles are used. This might just be a stripped-down version of bipole without e sources?
I'm fine with that. |
Sounds good. You think just loop-loop, or both, loop-loop and loop-dipole? As far as I know, in reality the magnetic source is always a loop, whereas the receiver can be both. As I mentioned, I'll be off from today for two weeks, but will have a look at this afterwards. Is it OK if I bother you for some testing/feedback once I have something? I personally have not much experience with magnetic sources, so it would be great and highly appreciated to have your input! |
Yes, most land-based TEM systems use loops as sources and coils or small loops as receivers, which is exactly what you suggest.
Yes, don't worry, it's absolutely ok. I'm glad to help. Have a nice time off! |
I hope to draft it early next week. I though of using
Does that make sense? Any clever idea for the function name? Simply |
It is necessary for frequency and time domain modelling, so I moved it to Now, given that, I actually think it is a bug in I'll have to dig into that in more detail, so don't expect it to resolve anytime soon. I don't want to include a workaround in |
Note to self to not forget: it should not be |
I am turning in circles. I take back what I said before. Again, it is the difference between a magnetic dipole and a loop source. So not a bug. Just confusing for someone never actually involved with magnetic sources. I am still confused with the units though... I will discuss this through and come back with a solution. |
@ruboerner , do you know of any analytical solutions for magnetic source with an electric receiver? What leaves me a bit uneasy with implementing a fix for |
Comparison with |
Extensive discussion, amongst others with the authors of the original article and their code EMmod, confirm that everything is fine with the current implementation. All sources and receivers are assumed to be dipoles. This changes for loops, and the solution therefore is really to have a |
Hi @ruboerner - This took more effort than I anticipated, but it is in. There is a new version
Furthermore, the following parameters are removed:
This means that it is a loop source, measured by
In the end I worked on a new branch, as everything changed a bit (see #48). I will therefore close this PR. I mentioned your help in the Credits-file. I would appreciate if you could test the new version and let me know if it works for you! Thanks. |
Here a graphical summary of it, with explanations: |
Hi @prisae , sorry for the late response. Big kudos for your effort you invested in implementing magnetic sources. This is indeed a huge improvement! |
Thanks @ruboerner . Did you test it, does it work work for you as expected? There is obviously much more that could be done in empymod for TEM-style things. Like allowing for more complicated waveforms, windowing as often used in TEM's, etc. But this is far off my current topic... But I'd be happy to include more things or assist in implementing them, if there is interest. |
Instead of using I added examples here: |
New example which goes into the same direction (the above mentioned examples are in the new sphinx-gallery too): |
With these small changes it seems possible to calculate the time-domain response of a layered half-space due to a magnetic dipole source. The reason for the code change is that prior to the f->t transform, the normalization (by i \omega \mu_0) has to be undone.