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

Add support for INGV DMX format #2452

Merged
merged 16 commits into from Dec 4, 2019
Merged

Add support for INGV DMX format #2452

merged 16 commits into from Dec 4, 2019

Conversation

ThomasLecocq
Copy link
Contributor

@ThomasLecocq ThomasLecocq commented Sep 13, 2019

What does this PR do?

Adds support to read INGV's DMX format

Why was it initiated? Any relevant Issues?

To rule the World & to read soooo many small DMX files from INGV Volcano Observatories, among others.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet): it is Closes DMX read #2438
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+DOCS"
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .

@ThomasLecocq ThomasLecocq added this to the 1.2.0 milestone Sep 13, 2019
@ThomasLecocq ThomasLecocq added the .io issues generally related to our read/write plugins label Sep 13, 2019
@ThomasLecocq ThomasLecocq added this to Waiting for Review in Release 1.2.0 Sep 13, 2019
Copy link
Member

@megies megies left a comment

Choose a reason for hiding this comment

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

See comments, also needs docs skeletons, see https://github.com/obspy/obspy/wiki/How-to%3A-add-a-new-submodule

obspy/io/dmx/__init__.py Outdated Show resolved Hide resolved
obspy/io/dmx/__init__.py Outdated Show resolved Hide resolved
obspy/io/dmx/__init__.py Outdated Show resolved Hide resolved
obspy/io/dmx/__init__.py Outdated Show resolved Hide resolved
obspy/io/dmx/core.py Outdated Show resolved Hide resolved
obspy/io/dmx/tests/test_core.py Outdated Show resolved Hide resolved
obspy/io/dmx/tests/test_core.py Outdated Show resolved Hide resolved
obspy/io/dmx/tests/test_core.py Outdated Show resolved Hide resolved
obspy/io/dmx/tests/test_core.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@megies
Copy link
Member

megies commented Sep 13, 2019

When making changes, would be good to rebase on current master in the process, to get cleaner CI results @ThomasLecocq

@ThomasLecocq
Copy link
Contributor Author

Ok boss :-)

@ThomasLecocq
Copy link
Contributor Author

@megies not sure I managed to rebase... please do it if you have 5min ?

@megies
Copy link
Member

megies commented Sep 16, 2019

Rebase was OK, I added some minor fixes and rebased on most recent master again

@megies
Copy link
Member

megies commented Sep 16, 2019

See comments, also needs docs skeletons, see https://github.com/obspy/obspy/wiki/How-to%3A-add-a-new-submodule

  • This still applies, docs skeletons missing, see wiki link above what needs be done. Also +DOCS to get a automatic build
  • the test file needs changing. 4MB is not acceptable here. I'm assuming you have some software to make a smaller DMX test file?
  • test cases never test the unpacked sample data, there should be a test checking like at least the first 5 samples and maybe the last as well.

@ThomasLecocq I force-pushed, so you will need to resync your local checkout

@ThomasLecocq
Copy link
Contributor Author

ThomasLecocq commented Sep 16, 2019

I have maybe forgotten to do the doc skeleton

Asking ingv for the test file

:toctree: autogen
:nosignatures:

bulletin
Copy link
Member

Choose a reason for hiding this comment

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

no such file I believe?

@megies
Copy link
Member

megies commented Sep 16, 2019

Commit history is messed up now, you merged branches together locally it seems, instead of hard resetting your local history to match the new history upstream after my last commit.

I'll fix it.

@megies
Copy link
Member

megies commented Sep 16, 2019

Cleaned up history, force-pushed. Also rebased the 4MB out of history

@ThomasLecocq
Copy link
Contributor Author

@megies thanks a lot and sorry for the mess :(

Why do tests fail ? nothing to do with dmx, right ?

@megies
Copy link
Member

megies commented Sep 17, 2019

I think the problem is that your _is_dmx routine is recognizing any given file as DMX. That while loop in there that tries to find a data block finishes just fine not finding anything and then goes to return True at end.

I don't understand tbh, assuming the file is not DMX, you are jumping to a supposedly next block position based on some random bytes parsed from the file? int(structtag.len_struct) + int(structtag.len_data) this position shift will contain random numbers if the file is not DMX.

I don't know the format but it looks like the block size can vary, so it will be impossible to check anything but the first data block on unknown files. Is it not possible to check something that always has to be there in the first data portion in the file?

@ThomasLecocq

@ThomasLecocq
Copy link
Contributor Author

hummmm.. OK, checking this now...

@megies megies moved this from Waiting for Review to In Progress in Release 1.2.0 Sep 18, 2019
@krischer
Copy link
Member

krischer commented Oct 9, 2019

@ThomasLecocq Any chance to get this done within the next view days?

@ThomasLecocq
Copy link
Contributor Author

yep :-)

@megies
Copy link
Member

megies commented Oct 25, 2019

ping @ThomasLecocq

@megies
Copy link
Member

megies commented Nov 27, 2019

Rebased and force-pushed @ThomasLecocq

@ThomasLecocq
Copy link
Contributor Author

Added some more clever checks in the _is_dmx method, let's see how CI like this...

@ThomasLecocq ThomasLecocq moved this from In Progress to Waiting on CI in Release 1.2.0 Nov 28, 2019
@ThomasLecocq ThomasLecocq moved this from Waiting on CI to Waiting for Review in Release 1.2.0 Nov 28, 2019
@megies
Copy link
Member

megies commented Nov 29, 2019

Rebased once more to get clean(er) CI results

@megies
Copy link
Member

megies commented Nov 29, 2019

@ThomasLecocq there are some problems on our minimum dependency build, maybe you can have a look?

https://travis-ci.org/obspy/obspy/jobs/618544621?utm_medium=notification&utm_source=github_status

http://tests.obspy.org/106920/#2

environment is listed here:
https://travis-ci.org/obspy/obspy/jobs/618544621#L1746

@megies megies moved this from Waiting for Review to In Progress in Release 1.2.0 Nov 29, 2019
@ThomasLecocq
Copy link
Contributor Author

ThomasLecocq commented Dec 2, 2019

@megies there is 1 unrelated error (next_fast_len missing in stack) and then

data = np.fromfile(fid, structtag_dtypes, 1)
IOError: first argument must be an open file

because the oooooold numpy is not happy... I'll check how this is handled in other formats... Or it's maybe due to my SpooledTemporaryFile ?

@megies
Copy link
Member

megies commented Dec 2, 2019

Or it's maybe due to my SpooledTemporaryFile ?

maybe.. not sure. we have some compat code for frombuffer i saw, maybe you get some ideas from that (although it doesnt allow specifying the object count atm). i would probably just recommend reading as many bytes as you want and use that compat layer from_buffer function.

io/mseed/util.py
232-    for file in files:
233-
234-        # If it's a file name just read it.
235-        if isinstance(file, (str, native_str)):
236-            # Read to NumPy array which is used as a buffer.
237:            bfr_np = np.fromfile(file, dtype=np.int8)
238-        elif hasattr(file, 'read'):
239-            bfr_np = from_buffer(file.read(), dtype=np.int8)
240-
241-        offset = 0
242-

Nevermind actually, I'll push the fix..

@megies
Copy link
Member

megies commented Dec 4, 2019

One single unrelated CI test fail, merging

@megies megies merged commit 1a0ca10 into master Dec 4, 2019
@megies megies deleted the feature_io_dmx branch December 4, 2019 11:53
@megies megies moved this from In Progress to Done in Release 1.2.0 Dec 4, 2019
@ThomasLecocq
Copy link
Contributor Author

Yeah !! Thanks a bunch for your help & time @megies !!! 🍻 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.io issues generally related to our read/write plugins
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

DMX read
3 participants