simpeg / aurora

software for processing natural source electromagnetic data
MIT License
14 stars 2 forks source link

Bug introduced - PKD test results out of bounds #156

Closed kkappler closed 2 years ago

kkappler commented 2 years ago

RR_original RR

Not 100% sure at what point this happened but the parkfield tests are not returning sensible TFs anymore. Attached figures show before and after TFs from the RR processing test.

Because tests are still passing, the synthetic data must still be returning practically exactly the same results as always (checked and confirmed) so this is likely an issue with calibration.

Note this is not a simple scale factor as the phases are also out of whack.

Solving this issue involves two things:

  1. Figure out what changed and repair it
  2. Create a test in tests/parkfield that ensures most results are at within say 10% of expected.
kkappler commented 2 years ago

hx_calibrated_spectra hx_calibrated_spectra

Updating the calibrated spectra in parkfield tests shows that the calibration of the pole-zero filters is corrupt

I will look into this. Working with main mt_metadata branch currently

kkappler commented 2 years ago

I rolled back aurora, mt_metadata and mth5 to Nov 12/13 2021 when aurora was tagged most recently and the bug is gone : aurora (Nov 12, 2021):

git checkout 2c30ccaa145bcc6531092e7c3c01844be6a90636

mt_metadata (Nov 12, 2021):

git checkout ab2a15b6c10acecfcb476e529ddcf166baf9c3ee 

mth5 (Nov 13, 2021):

git checkout 5550d751ad842cd8468ab6d0e3c08e14707746b9 

I will look for the bug this weekend

kkappler commented 2 years ago

Moving aurora forward to Nov 30 2021:

git checkout cd705a58905bbd108e5295dece5c618f12f2d60b

shows that the calibration is still correct. Ditto for December 22, 2021

git checkout 1c81c464fb2e01da58e2c6b361b406b117100dcf

Ditto for Feb 18 2022

git checkout 54510acfaa7c19a69e0805342ea863c382eed1d4

So, the issue doesn't seem to be related to a change in aurora, but rather some change in mt_metadata or mth5.

kkappler commented 2 years ago

Now, with aurora set to the latest commit on main, try bumping mt_metadata to Nov 24, 2021:

git checkout 09c0e755404bc189656015c50c6207e279c1b2cf

Still looks good.

kkappler commented 2 years ago

When bumping mt_metadata to Dec 8, 2021

git checkout 1d0f398e0122a3a2f4f60732ac7b2b7c6ab8c7d4

I get an exception, presumably due to needing to update mth5 ... :

2022-03-06 10:23:57,590 [line 135] mth5.setup_logger - INFO: Logging file can be found /home/kkappler/software/irismt/mth5/logs/mth5_debug.log
Warning dt not defined, using dt=1
Warning dt not defined, using dt=1
Warning dt not defined, using dt=1
Warning dt not defined, using dt=1
Warning dt not defined, using dt=1
channel_keys: ['ex', 'ey', 'hx', 'hy', 'hz']
calibrating channel ex
Traceback (most recent call last):
  File "/home/kkappler/software/irismt/mth5/mth5/groups/filters.py", line 43, in __init__
    self.zpk_group = ZPKGroup(self.hdf5_group.create_group("zpk"))
  File "/home/kkappler/anaconda2/envs/py37/lib/python3.7/site-packages/h5py/_hl/group.py", line 65, in create_group
    gid = h5g.create(self.id, name, lcpl=lcpl, gcpl=gcpl)
  File "h5py/_objects.pyx", line 54, in h5py._objects.with_phil.wrapper
  File "h5py/_objects.pyx", line 55, in h5py._objects.with_phil.wrapper
  File "h5py/h5g.pyx", line 162, in h5py.h5g.create
ValueError: Unable to create group (name already exists)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "test_calibrate_parkfield.py", line 52, in <module>
    main()
  File "test_calibrate_parkfield.py", line 48, in main
    test()
  File "test_calibrate_parkfield.py", line 44, in test
    validate_bulk_spectra_have_correct_units(run_obj, run_ts_obj)
  File "test_calibrate_parkfield.py", line 28, in validate_bulk_spectra_have_correct_units
    include_decimation=True,
  File "/home/kkappler/software/irismt/aurora/tests/parkfield/calibration_helpers.py", line 74, in parkfield_sanity_check
    pz_calibration_response = channel.channel_response_filter.complex_response(
  File "/home/kkappler/software/irismt/mth5/mth5/groups/master_station_run_channel.py", line 1664, in channel_response_filter
    self.hdf5_dataset.parent.parent.parent.parent["Filters"]
  File "/home/kkappler/software/irismt/mth5/mth5/groups/filters.py", line 45, in __init__
    self.zpk_group = ZPKGroup(self.hdf5_group["zpk"])
  File "/home/kkappler/software/irismt/mth5/mth5/groups/filter_groups/zpk_filter_group.py", line 31, in __init__
    super().__init__(group, **kwargs)
  File "/home/kkappler/software/irismt/mth5/mth5/groups/base.py", line 89, in __init__
    self._initialize_metadata()
  File "/home/kkappler/software/irismt/mth5/mth5/groups/base.py", line 137, in _initialize_metadata
    self._metadata = Base()
  File "/home/kkappler/software/irismt/mt_metadata/mt_metadata/base/metadata.py", line 49, in __init__
    self._set_attr_dict(attr_dict)
  File "/home/kkappler/software/irismt/mt_metadata/mt_metadata/base/metadata.py", line 92, in _set_attr_dict
    setattr(self, k, value)
UnboundLocalError: local variable 'value' referenced before assignment
kkappler commented 2 years ago

Searched around a little bit to find a stable pair of mt_metadata and mth5 commits that don't raise exception. Found that December 17, 2021 was stable

For mt_metadata:

git checkout 931745b16d400e186a7ef8c655a0fe67aee5c453

and for mth5:

git checkout f8d9906418d2b76ef17ec5ba1fc50c1f01d593d4

hx_calibrated_spectra

Something is wrong with the calibrated spectra. The difference is not just s scale factor, which would make the red and blue curves in the attached figure translate only in the vertical direction. The actual shapes of the curves differ, if only subtly as well. This may have to do with a change in the handling of poles and zeros in mt_metadata is my current best guess

kkappler commented 2 years ago

TESTED DOES NOT HAVE BUG: mth5 Nov 20, 2021 c11a9e034a42b5527f7a8f9495d03f62a92286f0 mt_metadata Nov 24 2021 09c0e755404bc189656015c50c6207e279c1b2cf mt_metadata Nov 29 2021 e8c2d91b1c5b7f63279ed7dac19279d56cfe3af7 mt_metadata Nov 30 2021 28af661703dfa40fde0861d6c63620d3237b4c7b

So, the current state of debugging suggests that the problem was introduced after Nov 30, 2021 in mt_metadata or after Nov 20 in mth5, but before Dec 17, 2021 in either mt_metadata or mth5.

kkappler commented 2 years ago

TESTED and confirmed bug-free mth5 Nov 22, 2021: 53b484344685597b0e039c1bcfb9bf28423d86eb mt_metadata Nov 30, 2021 28af661703dfa40fde0861d6c63620d3237b4c7b

I have now tested the latest pairs where all tests internally pass in mth5 and mt_metadata. The following tests will look closer for the changes, but may contain internal errors in mth5 and mt_metadata

kkappler commented 2 years ago

TESTED and confirmed this calibration bug is not an issue mth5 Nov 22, 2021: ddabb123e01cf65c014f7576b29f9c9948865bba mt_metadata Nov 30, 2021 28af661703dfa40fde0861d6c63620d3237b4c7b

TESTED and confirmed this calibration bug IS an issue mth5 Dec 17, 2021: bffc05b5eae3132ae9163e89859d9cc857c9c93c mt_metadata Nov 30, 2021 28af661703dfa40fde0861d6c63620d3237b4c7b

kkappler commented 2 years ago

@kujaku11 Looks like this problem has been isolated to the commit on Dec 17, 2021 with the comment:

updated dtype in zpk to complex

here is a link to the commit.

The file is mth5/groups/filter_groups/zpk_filter_group.py

Can you take a look at the numerical behaviour here? Maybe make a test of the output before and after the change ... I think that the pole-zero representation of the filter is getting corrupted somehow

kujaku11 commented 2 years ago

Thanks for isolating this. I'll have a look this afternoon.

On Sun, Mar 6, 2022, 11:16 AM kkappler @.***> wrote:

@kujaku11 https://github.com/kujaku11 Looks like this problem has been isolated to the commit on Dec 17, 2021 with the comment:

updated dtype in zpk to complex

here is a link https://github.com/kujaku11/mth5/commit/bffc05b5eae3132ae9163e89859d9cc857c9c93c to the commit.

The file is mth5/groups/filter_groups/zpk_filter_group.py https://github.com/kujaku11/mth5/commit/bffc05b5eae3132ae9163e89859d9cc857c9c93c#diff-8d7039232bc1039727ad8a6def0b48c7290b9b0e114ec0a045c0b5fe4b80acc9

Can you take a look at the numerical behaviour here? Maybe make a test of the output before and after the change ... I think that the pole-zero representation of the filter is getting corrupted somehow

— Reply to this email directly, view it on GitHub https://github.com/simpeg/aurora/issues/156#issuecomment-1060022246, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQA5BTQY4YWC4GDMOAJCCTU6T77XANCNFSM5P7AL2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

kujaku11 commented 2 years ago

@kkappler I can confirm the bug you found. There were a couple of issues:

  1. The change to storing poles and zeros in HDF5 using h5py from an 2 x N matrix with columns for real and imaginary to complex changed how older files are read in. Basically they weren't, so you got a ZPK filter with no poles or zeros. That's been fixed in the MTH5 pull request 83.
  2. Another bug that took me a while to find was that when a pole or zero was identically 0 it was not propagated through to a filter object. There is some logic in mt_metadata when an array is all 0's its assumed to be null. Anyway when the complex response was computed it was wrong. This has been fixed in mt_metadata pull request 79.

@kkappler can you confirm on your end that this issue has been resolved with:

Once you confirm I will accept the pull requests into the main branches.

kkappler commented 2 years ago

@kujaku11 I confirm that when I pull and switch mth5 to fix_filters, and mt_metadata to updates the bug appears to be gone:) hx_calibrated_spectra SS

I put a couple of tests on my current branch (aurora issue31) that will flag this sort of error in future and next time I merge into main they'll be active.

kujaku11 commented 2 years ago

@kkappler Nice, I'll close this issue and let you know when I merge both branches into their respective mains.