obspy / obspy

ObsPy: A Python Toolbox for seismology/seismological observatories.
https://www.obspy.org
Other
1.15k stars 530 forks source link

Incorporate new evalresp version? #1493

Open krischer opened 8 years ago

krischer commented 8 years ago

In #1492 it was noticed that a new evalresp version has been out for quite a while:

http://ds.iris.edu/ds/nodes/dmc/software/downloads/evalresp/4-0-6/revisions/

We should probably investigate if we want these changes. The main new feature is stationxml support which we already have. If we want to use the new stationxml support in evalresp we would once again have to use temporary files which I would really rather not do...

Additionally we would then have a compile time dependency on libxml2 - we already have a dependency on lxml which also needs libxml2 so in principle this is no problem but we'd probably run into lots of build issues on all the various platforms.

If we don't incorporate the new evalresp we might have to backport this changlog item:

* MODIFIED: Handling of stages with “no data”. Now accepted, as they may still have decimation and channel gain.

All in all I think it would be nice have an up-to-date evalresp version within ObsPy but the effort is non-trivial as our evalresp version is patched quite a bit. I was actually under the impression that evalresp is basically not developed anymore but onyl receives the most necessary patches.

What do you guys think?

ghisvail commented 7 years ago

Now that the issue with the vendored libmseed is solved, the vendored evalresp also must be made substitutable for a system installed version in order to make obspy package-able on Linux distributions.

Based on your listing here, I see two issues:

  1. The vendored evalresp version is outdated. Linux distributions would typically package the latest upstream version, unless there is a very good reason not to.
  2. The vendored evalresp is patched, i.e. there are (significant?) deviations from the upstream code to tailor it to what obspy expect.

Perhaps solving 1. would make 2. a little easier?

krischer commented 7 years ago

Perhaps solving 1. would make 2. a little easier?

Unfortunately not - it would very likely still require quite a number of patches and it is a lot of work for us with no true benefits. The main issue is that evalresp is not designed to be usable as a library where one can directly pass the information as C structs and we had to make some changes to enable that.

Please see my comment in https://github.com/obspy/obspy/issues/1492#issuecomment-285618905 about the upcoming re-design of evalresp. I don't know how long that will take though and if it will truly work for us - but I think that is one of the goals of the re-design.

megies commented 7 years ago

Hmm.. IMHO we should focus on more important things, e.g. get 1.1.0 out. In principle it would be nice to get rid of our hacky low-level use of evalresp. But in practice it works and we've rigorously tested it. And especially if there is massive changes ahead upstream with that redesign.. I would rather wait and sit tight and wait how it evolves and focus work on other things.

As to the packaging issues @ghisvail has brought up.. I think removing all vendored codes and making obspy work with separately packaged versions of these codes is really a major issue that will take lots of work (if it is even possible at all). I'm really impressed with @QuLogic's efforts of working with external libmseed packages, but again.. I think we should focus on obspy core issues. Our changelog for 1.1.0 is already 108 lines, and we have tons of PRs open that still need handling for 1.1.0. Long story short: We're already sorely outgunned tackling our core development.. and opening more battlefields doesn't seem like a good idea, at least to me..

QuLogic commented 7 years ago

I don't think there's anything to do WRT external packaging; I've already done that work a long time ago for both libmseed and evalresp.

I took a look at evalresp 4.0.6, and it's already greatly refactored. If there's going to be another refactoring, I don't think it makes much sense to upgrade until that's done.

ghisvail commented 7 years ago

I would rather wait and sit tight and wait how it evolves and focus work on other things.

If the coupling between the patched evalresp and obspy is that tight, then indeed this endeavour will be a major effort.

I don't think there's anything to do WRT external packaging

Depends of the target audience of the packaging. For a personal archive (i.e. Ubuntu PPA or Fedora COPR), the vendoring could be totally kept.

However, for official inclusion to a distro (i.e. Debian / Ubuntu or Fedora), there are much stricter criteria, including the ability to substitute vendored dependencies for their system counterparts.

AFAIC, I am totally fine if this never happens.

markcwill commented 7 years ago

A couple thoughts and questions. Forgive my ignorance:

  1. libmseed is a standard-style library that lends well to wrapping, it does what it says on the label: read/write miniSEED. evalresp seems a bit more complicated. At a glace, it seems more like the rdseed program; a randomly maintained "ball-of-mud" project rolling on momentum generated by previous investment and whoever might have a vested interest. I think a plan should take into account whether this is being patched occasionally just because people use it, or a major supported refactor is in the works. As @krischer notes, the download on the IRIS site is 2 years old.
  2. I see no licensing for evalresp, so I'm assuming that's one of the problems, do we know the license? And does that prevent us from using evalresp code in a derivative work?
  3. Given the above, I think that reproducing evalresp functionality within obspy using existing dependencies (i.e. numpy) or as a compiled extension would be the best long-term solution (I think this is what @megies is saying). Is there any reason (other than convenience) to use evalresp? Is just creating our own obspy.resp module an option, similar to obspy.taup? And does that solve the Debian vendoring issue?
ghisvail commented 7 years ago

reproducing evalresp functionality within obspy using existing dependencies (i.e. numpy) or as a compiled extension [...] does that solve the Debian vendoring issue?

Indeed, it would.

krischer commented 7 years ago

I see no licensing for evalresp, so I'm assuming that's one of the problems, do we know the license? And does that prevent us from using evalresp code in a derivative work?

See https://github.com/obspy/obspy/pull/827

Given the above, I think that reproducing evalresp functionality within obspy using existing dependencies (i.e. numpy) or as a compiled extension would be the best long-term solution (I think this is what @megies is saying). Is there any reason (other than convenience) to use evalresp? Is just creating our own obspy.resp module an option, similar to obspy.taup? And does that solve the Debian vendoring issue?

See #1718

Thanks for your comment :) For some reason I never seriously considered that!