sixty-north / segpy

A Python package for reading and writing SEG Y files.
Other
99 stars 54 forks source link

Use plugins and named extensions for trace headers #83

Open abingham opened 6 years ago

abingham commented 6 years ago

After running segpy over all of the SEGY files we've got, I've a better sense of why we need to let users specify different trace headers in different situations. Our new, strict header definitions don't work for some of our SEGY, but slightly looser definitions would work. So I was thinking about how to let users define and select which header definition to use.

One approach that worked well for cosmic ray is plugins and named extensions. In short, each header definition would be registered (e.g. using stevedore) with a given name, and these extensions would be provided by plugins. We could package several with segpy itself: Rev1-strict, Rev1-relaxed, etc. Stevedore would allow users to provide their own if they wanted/needed.

The other part of the puzzle would be deciding how we let them select a header. One option would be on the command line, another would be with a config file, and we could also combine these approaches if that made the most sense.

I just wanted to get this idea out there. It's worked pretty well for Cosmic Ray, so it might be appropriate for segpy as well.

rob-smallshire commented 6 years ago

create_reader() and write_segy() already accept an optional trace_header_format argument. It should be a simple matter to have them also accept an optional binary_reel_header_format` argument.

It's not obvious to me why a plugin mechanism is necessary though. Segpy is designed to be used as a library rather than a framework, so whatever executive program invokes Segpy is responsible for handing it appropriate header format definition classes (or at least knowingly accepting the defaults). This executive program could be one of the command-line programs (although the ones we ship are just illustrative examples), or could be some more elaborate application.

This is the opposite of Cosmic-Ray because in that case, it is the executive program (and framework), and so needs to have the means to discover and invoke extensions itself.

In summary, I'm not sure why we can't just include various header format definition classes as we already do, and allow clients to specify them when they invoke the API. If client programs need automatic discoverability for header format definition classes they can layer that on top of the capabilities that Segpy already provides, without Segpy needing to take on position on the mechanism used.

Of course, it's possible I've overlooked some important use-case!

rob-smallshire commented 6 years ago

What's the failure mode when Segpy fails because of an incompatible header definition. Do you get a reasonable error, or something internal and obscure?

abingham commented 6 years ago

IIRC, it was pretty reasonable. It complained about a value not being in the domain of the header field. But I also recall that it was couched in some nested set of exceptions, so it could be cleaner. (I ran this all at home a week or so ago, but I'll try to re-run it again to give better feedback).

Regarding the overall motivation for this issue: if the tools we're shipping are just examples, then you're right that the plugin idea is over-engineered. With that said, the tools we provide for reading metadata could be generally useful. Maybe what I should really be advocating for is a separate tool built on segpy that provides the kind of header flexibility I'm talking about.

On Tue, Feb 6, 2018 at 10:32 AM Robert Smallshire notifications@github.com wrote:

What's the failure mode when Segpy fails because of an incompatible header definition. Do you get a reasonable error, or something internal and obscure?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sixty-north/segpy/issues/83#issuecomment-363363690, or mute the thread https://github.com/notifications/unsubscribe-auth/AAE1eOJmplINM04TK72YMK2-YPA3dglNks5tSBw_gaJpZM4R6ncU .