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

Rotations fixed #111

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from
Open

Rotations fixed #111

wants to merge 23 commits into from

Conversation

kujaku11
Copy link
Collaborator

Rotation of the impedance and tipper were correct in the original versions. Plotting schemes were not and have been patched. All plotting schemes should now be coherent.

Description

Changed all the plotting schemes to now have a rotation_angle property that will rotate the impedance tensor and induction vectors clockwise positive.

Most of the plotting tools, especially polar plots assume positive counterclockwise and 0 = E. Adapted the plotting schemes to adhere to this by 90-strike.

Motivation and Context

Should have made the rotations consistent across the plotting schemes

How Has This Been Tested?

Have a test data set with a conductive square in a half space and made sure the ellipse and induction arrows were oriented properly. Visual tests, someone should verify.

Screenshots (if appropriate):

Types of changes

  • [x ] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [x ] My code follows the code style of this project.
  • My change requires a change to the documentation.
  • [x ] I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@kujaku11 kujaku11 requested review from zhang01GA and alkirkby May 12, 2020 01:26
@zhang01GA
Copy link
Contributor

Thanks Jared for fixing the rotation issue. I am OK with merging.
Hi @alkirkby , if you have completed the correctness-testing?

Copy link
Contributor

@zhang01GA zhang01GA left a comment

Choose a reason for hiding this comment

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

I am OK with the merging request if @alkirkby completed the correctness-testing.

@alkirkby
Copy link
Contributor

I'm sorry I haven't yet, will get to this today and then merge if all is good.

@alkirkby
Copy link
Contributor

I get this error when I try to run mtpy/examples/scripts/plot_phase_tensor_maps.py:
Traceback (most recent call last):

File "C:\mtpywin\mtpy\examples\scripts\plot_phase_tensor_map.py", line 15, in
import mtpy.imaging.phase_tensor_maps as pptmaps

File "C:\mtpywin\mtpy\mtpy\imaging\phase_tensor_maps.py", line 26, in
import mtpy.imaging.mtplottools as mtpl

File "C:\mtpywin\mtpy\mtpy\imaging\mtplottools.py", line 17, in
from mtpy.core import mt

File "C:\mtpywin\mtpy\mtpy\core\mt.py", line 20, in
import mtpy.analysis.pt as MTpt

File "C:\mtpywin\mtpy\mtpy\analysis\pt.py", line 28, in
class PhaseTensor(object):

File "C:\mtpywin\mtpy\mtpy\analysis\pt.py", line 328, in PhaseTensor
@z_err.setter

AttributeError: 'function' object has no attribute 'setter'

Copy link
Contributor

@alkirkby alkirkby left a comment

Choose a reason for hiding this comment

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

I seem to get this error below quite a bit, this should be fixed before we merge to develop. Let me know if you need the underlying code (I got a similar error when trying to run the phase tensor maps example in the examples/scripts directory

File "C:\mtpywin\mtpy\mtpy\analysis\pt.py", line 28, in
class PhaseTensor(object):

File "C:\mtpywin\mtpy\mtpy\analysis\pt.py", line 328, in PhaseTensor
@z_err.setter

AttributeError: 'function' object has no attribute 'setter'

@kujaku11
Copy link
Collaborator Author

kujaku11 commented May 21, 2020 via email

@alkirkby
Copy link
Contributor

alkirkby commented May 21, 2020

OK great, that works now. The strike still seems to be repeated in the 1st and 4th quadrants on the strike plots, is this correct?
image

Will look into the phase tensor plots now.

@alkirkby
Copy link
Contributor

Would you be able to update the examples to show how to use the new rotation_angle property? I'm having trouble using it. I tried setting a rotation_angle property in examples/scripts/plot_phase_tensor_maps.py but it is giving me this error:

File "C:\mtpywin\mtpy\mtpy\imaging\phase_tensor_maps.py", line 518, in init
self.plot()

File "C:\mtpywin\mtpy\mtpy\imaging\phase_tensor_maps.py", line 834, in plot
**self.kwargs)

File "C:\W10DEV\Anaconda3\envs\mtpywin\lib\site-packages\matplotlib\patches.py", line 1394, in init
Patch.init(self, **kwargs)

File "C:\W10DEV\Anaconda3\envs\mtpywin\lib\site-packages\matplotlib\patches.py", line 96, in init
self.update(kwargs)

File "C:\W10DEV\Anaconda3\envs\mtpywin\lib\site-packages\matplotlib\artist.py", line 1006, in update
ret = [_update_property(self, k, v) for k, v in props.items()]

File "C:\W10DEV\Anaconda3\envs\mtpywin\lib\site-packages\matplotlib\artist.py", line 1006, in
ret = [_update_property(self, k, v) for k, v in props.items()]

File "C:\W10DEV\Anaconda3\envs\mtpywin\lib\site-packages\matplotlib\artist.py", line 1002, in _update_property
.format(type(self).name, k))

AttributeError: 'Ellipse' object has no property 'rotation_angle'

@alkirkby
Copy link
Contributor

I am also having trouble using the orthogonal and fold options in the strike roseplots, but that is a separate issue I think (I get the same issue in develop)

@kujaku11
Copy link
Collaborator Author

kujaku11 commented May 25, 2020 via email

@alkirkby
Copy link
Contributor

Another thing I noticed is if you plot phase tensors using rotated edis, the phase tensors rotate by the rotation angle of the edi's. I guess this is what the rotation_angle property is for, to rotate it back? We should probably get the code to read in the rotation angle from the edi files and rotate back to zero for plotting.

@kujaku11
Copy link
Collaborator Author

kujaku11 commented May 26, 2020 via email

@alkirkby
Copy link
Contributor

alkirkby commented May 26, 2020

Sounds great, I will test it now.
Yeah I think the default should be to rotate back so that X is north and Y is east because this is the coordinate system the ellipses are plotted in. If we leave a rotation of say 30 degrees, the impedance values in the edi are relative to X = 30 degrees east of north so the phase tensor will be shown incorrectly if it's plotted with the X axis at 0 degrees (North). Does that sound right to you or am i missing something?

The information on what north is relative to in the data should be in the edi so can we get that out too? Usually it's geographic north I think. Our plots are generally shown with geographic north vertical, so I guess the data should be rotated so that the north axis in the edi matches north in the plot?

@kujaku11
Copy link
Collaborator Author

kujaku11 commented May 26, 2020 via email

@alkirkby
Copy link
Contributor

Yeah, I guess there should be a plot_geographic option. But regardless of how the data are rotated, I thought the phase tensor azimuth should always be the same relative to north? If the data are rotated by 30 degrees the phase tensors should plot relative to the 30 degree axis, shouldn't they, so they should look the same as unrotated data would look like plotted normally? It doesn't make sense to me that the rotation angle should affect how they plot?

@alkirkby
Copy link
Contributor

I guess if the reference is not recorded in the edi file we have to assume something, and geographic north is probably our best bet in terms of a default.

@alkirkby
Copy link
Contributor

alkirkby commented May 27, 2020

OK almost there with testing. I just tried the rotation_angle property in /examples/scripts/plot_phase_tensor_section.py and when I set the rotation_angle property I get an error:

from mtpy.imaging.phase_tensor_pseudosection import PlotPhaseTensorPseudoSection
import os.path as op
import os

path to edis

edi_path = r'C:\mtpywin\mtpy\examples\data\edi_files_2'

save path

savepath = r'C:\tmp'

edi list

elst=[op.join(edi_path,edi) for edi in os.listdir(edi_path) if ((edi.endswith('.edi')))]# and edi.startswith('GB')

create a plot object

plotObj = PlotPhaseTensorPseudoSection(fn_list = elst,
linedir='ns', # 'ns' if the line is closer to north-south, 'ew' if line is closer to east-west
stretch=(17,8), # determines (x,y) aspect ratio of plot
station_id=(0,10), # indices for showing station names
plot_tipper = 'yri', # plot tipper ('y') + 'ri' means real+imag
font_size=5,
lw=0.5,
rotation_angle=0,
ellipse_dict = {'ellipse_colorby':'skew_seg',# option to colour by phimin, phimax, skew, skew_seg
'ellipse_range':[-12, 12, 3]} # set color limits - default 0,90 for phimin or max,
# # [-12,12] for skew. If plotting skew_seg need to provide
# # 3 numbers, the 3rd indicates interval, e.g. [-12,12,3]
)

update ellipse size (tweak for your dataset)

plotObj.ellipse_size = 2.5

plotObj.plot()

Traceback (most recent call last):

File "C:\Users\u64125\OneDrive - Geoscience Australia\AusLAMP_NSW\mtpy_test\plot_phase_tensor_section_2.py", line 33, in
'ellipse_range':[-12, 12, 3]} # set color limits - default 0,90 for phimin or max,

File "C:\mtpywin\mtpy\mtpy\imaging\phase_tensor_pseudosection.py", line 403, in init
for key, value in kwargs:

ValueError: too many values to unpack (expected 2)

@kujaku11
Copy link
Collaborator Author

kujaku11 commented May 28, 2020 via email

@kujaku11
Copy link
Collaborator Author

kujaku11 commented May 28, 2020 via email

@alkirkby
Copy link
Contributor

Cool, yep that sounds like a good approach. If the correct metadata is recorded in the edi's, the datum for the rotation should be there. But yes, that's not always the case.

And yeah, that's my understanding with rotations too. And since all the map plotting functions in MTPy (correct me if I'm wrong) have geographic north as vertical on the plot, we should always "correct" or rotate the data so that the X axis of the impedance tensor is also vertical. If the data have north as geomagnetic north, they need to be rotated back to geographic north before plotting...

So if the user feeds in rotated edi files, we need to "correct" them so that north is vertical. Your approach described above would do that, so I think that's the best way (with plenty of warnings saying that's what we're doing).

@zhang01GA
Copy link
Contributor

Hi @alkirkby and @kujaku11 Recently we have switched to Python-3 auto unit-testing in Github + Travis CI. So could you please merge the updated "develop" into your branch "rotation_test" and then run pytest locally in your machine using Python-3 environment. I have done such a test in my Ubuntu18.04 with Anaconda virtual env Python 3.7.6.

It would be great if you can investigate and fix the unit tests before merge.

Run pytest -v --ignore=tests/SmartMT tests

Observed 4 failed unittests:

=========================== short test summary info ============================
FAILED tests/analysis/test_geometry.py::Test_Geometry::test_get_eccentricity_from_edi_file
FAILED tests/core/test_z.py::TestZ::test_rotate - AssertionError: False is no...
FAILED tests/modeling/test_occam2d.py::TestOccam2D::test_fun - ValueError: ca...
FAILED tests/modeling/ModEM/test_modem_inputfiles_builder.py::TestModemInputFilesBuilder::test_fun_rotate

===== 4 failed, 188 passed, 6 skipped, 3179 warnings in 1498.80s (0:24:58) =====

@kujaku11
Copy link
Collaborator Author

kujaku11 commented Jun 1, 2020 via email

@kujaku11
Copy link
Collaborator Author

kujaku11 commented Jun 1, 2020 via email

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