Unnessary information when writing to a SACPZ file

[ziyixi]

• Please check whether the bug was already reported or fixed.
  No
• Please provide the following information:
  • ObsPy version, Python version and Platform (Windows, OSX, Linux ...)
    obspy 1.2.2, Python 3.8.5, Linux
  • How did you install ObsPy and Python (pip, anaconda, from source ...)
    anaconda
• issue
  

  obspy/obspy/io/sac/sacpz.py

  Line 101 in d56e8eb

  out.append("* INSTTYPE : %s" % cha.sensor.type)

I am planning to write to a SACPZ file while cha.sensor==None which means it has no sensor information, the program will crash since None.type doesn't exit. The general purpose for us to use a SACPZ file is to remove the instrumental response and the sensor information might not be so essential. So maybe we can add an if flag here to see whether cha.sensor exist or not?

[megies]

cha.sensor and cha.sensor.type or "---" should do the trick

[jmunoz94]

Hello, @megies . I am wondering if this is something that you still want to address, considering that 2+ years have passed since it was reported. If yes, may I work on this? (I think no explicit assignment is required, but since I am asking other questions, I thought I could ask that, too).

Now, related to the change itself: would a try... except... approach work for you or would you prefer a boolean approach like the one you suggest?

I was thinking on something like this:

try:
    out.append(f"* INSTTYPE    : {cha.sensor.type}")
except AttributeError:
    out.append("* INSTTYPE    : ---")

May I also change %s and .formats for f-strings (wherever possible in this file, at least) to make the PR a bit less simple?

[megies]

Feel free to go ahead. But your try/except doesn't handle all cases, it will print "None" in output if sensor is set but its type is None.

f"* INSTTYPE : {cha.sensor and cha.sensor.type or ''}" handles everything and in a more elegant way, I think.

If you want to work on this we will also need tests for it, not sure if it's worth the hassle, considering that SACPZ is a format that probably very few people use, with how widespread StationXML has become the default.

[jmunoz94]

Oh, that is true. I was only considering the case described in the issue (i.e. sensor being None).

I will give this a try this weekend and update when I am done or if I give up. Thank you!

[jmunoz94]

Hey, @megies: I have a feature branch with a potential fix to this (including a new test).

The branch is fix_2707_missing_sensor_or_sensor_type in my fork. The contribution guidelines suggest asking/contacting you in the issue for you to convert it to a PR. Could you help me with that? Thank you.

[megies]

The contribution guidelines suggest asking/contacting you in the issue for you to convert it to a PR. Could you help me with that?

We actually do not do this anymore, since sadly GitHub never officially added this workflow and the existing implementation Github has had for years on end now makes it so whoever converts the issue to a PR will "hijack" the first post authorship, which is kinda confusing.

Long story short, just open a PR @jmunoz94

[jmunoz94]

Done @megies. I do not seem to have permissions to add labels, but it is ready for review.