basic_text_parser.to_event_fields throws an error when get_field_type returns None

[jmonticolo]

https://github.com/INGV/qquake/blob/8d193b0f4b5be455d343342bb10e2a0f57419154/qquake/basic_text_parser.py#L152

If the field type isn't found, basic_text_parser.get_field_type returns None and QgsField("some field name", None) throws a TypeError.

tested on 1.0.1

[MarioLocati]

The function cannot generate such an error because it is used to parse the built-in configuration which is carefully tested before publication, so there is no need to add any safety procedure. @nyalldawson could you please confirm that?

[fabienengels]

Hi Mario,

Unfortunately, you can't rely on this first line as there is no real header defined in the text output standard. This line is not mandatory and its content it's not defined by the standard as the documentation says :

This simple text output format contains one event per line with common fields separated by vertical bar characters (ASCII decimal 124).  Field entries cannot contain vertical bar characters.  Lines beginning with a hash character (“#”: ASCII decimal 35) should be considered comment lines.

But the number of columns is defined so maybe it would be easier to simply retrieve values by index are they are defined by the standard (even a empty value should be represented by an empty field).

We really need to work on an GeoJSON output standard for the fdsn-event (we made one available on our new webservice and it makes life so easier) and one for the macroseismic data :)

[MarioLocati]

@fabienengels In my experience fdsnws-event compliant services always report exactly the same header, the one defined in the official documentation. Below an extract of the current version 1.2 (2019-06-27) https://www.fdsn.org/webservices/fdsnws-event-1.2.pdf

So far, the only fdsnws-event implementation I found that do not provide a text output is ISC (http://www.isc.ac.uk/fdsnws/event/1/).

About the GeoJSON output it is currently a mess, any organisation is using its own implementation. As no standard is defined for that, there is no chance that it could be used in a tool like QQuake. Yes, the community should definitely work on that.

Meanwhile, feel free to add your (compliant) services into https://github.com/INGV/qquake/blob/master/qquake/config/config.json

[fabienengels]

Even if a lot of webservices show this particular header, it doesn't mean is part of the standard (and many of them use the same engine). If you look closer to the page you show, there is no reference to a header, just an example of lines with what you should expect in these fields. So why not simply access to the value by the column index ? I think that'll be a stronger way to do it as the order and number of columns is well defined by the standard. That would make qquake working with our webservice and the UK one (even I'll end up changing our first line to match what you expect).

(I know, right know GeoJSON is a mess in the community, hopefully there will be an another meeting in Milan to define a good one at least for the macroseismic part at almost everything need to be build :) ).

I can contribute to the config file but I'm not sure to understand why the accepted parameters are defined in this file. Accepted parameters of the webservice should be queried through the wadl file provided by the webservice (ex : https://api.franceseisme.fr/fdsnws/event/1/application.wadl ). As tomorrow we can support new parameters (not yet implemented), querying the wadl avoid the need to update a config file for the apps using the webservice, this is what Obspy client do to know what parameters are supported.

[MarioLocati]

We'll solve the issue with the header as soon as possible.

We are aware of the wadl description of the services, but we were forced to end up with a reduced number of features due to the limited allocated development resources. The current config.json is precisely a proxy of the missing wadl support that couldn't make it. In fact, we consider the current version of QQuake a test, if it will succeed (we are monitoring the downloads), we have already planned the support of the wadl, multiple and customization of styles, CQL support for the OGC services, and many other features.

[fabienengels]

I see, I'll try to make a Pull Request in the coming weeks.

[MarioLocati]

I am -quite slowly- learning a bit of Python and how to use GitHub by myself. Did a first attempt in fixing the code in the file basic_text_parser.py. Please, check below three changes I would like to apply that end up fixing the code when using your fdsnws-event, let me know what do you think.

First change

EVENT_FIELD_TYPE = {
    'EventID': QVariant.String,
    'Time': QVariant.DateTime,
    'Latitude': QVariant.Double,
    'Longitude': QVariant.Double,
    'Depth/km': QVariant.Double,
    'Depth/m': QVariant.Double,
    'Author': QVariant.String,
    'Catalog': QVariant.String,
    'Contributor': QVariant.String,
    'ContributorID': QVariant.String,
    'MagType': QVariant.String,
    'Magnitude': QVariant.Double,
    'MagAuthor': QVariant.String,
    'EventLocationName': QVariant.String,
    'EventType': QVariant.String,
    'MagnitudeType': QVariant.String,
    'MagnitudeAuthor': QVariant.String
}

Second change

def parser_header_line(self, line):
    assert line[0] == '#'
    line = line[1:]
    headers = [h.strip() for h in line.split('|')]

    for h in headers:
        h = h.strip()
        if h == 'Depth/km' and self.depth_unit == QgsUnitTypes.DistanceMeters:
            h = 'Depth/m'
        if h == 'MagnitudeType':
            h = 'MagType'
        if h == 'MagnitudeAuthor':
            h = 'MagAuthor'
        self.headers.append(h)

Third change

def to_event_fields(self, selected_fields=None):
    fields = QgsFields()
    for f in self.headers:
        if self.get_field_type(f):
            fields.append(QgsField(f, self.get_field_type(f)))

    return fields

[MarioLocati]

I have submitted the above code to the main branch as to me it is working just fine https://github.com/INGV/qquake/commit/62b68f0bb2e7847b0f53dbdc301038ded6ea257a#diff-0020ad9e10a4a19cad2470f1604592535488902d4f95c936c2696451ecb83dce

[fabienengels]

Hi Mario,

That will just fix for our webservices not the UK one while its respects the standard. Get the field per index, this is the strongest solution and simplest solution.

[MarioLocati]

OK, will try to do that. Could you please provide a link to the fdsnws-event UK service?

[ghtmtt]

@nyalldawson what is your opinion? Grabbing each header from the different service providers could have a heavy impact on the rest of the code?

[MarioLocati]

Thanks to @GiorgioMariaDeSantis we have implemented the loading of the text output of the fdsnws-event using index. The solution is already in the main branch, and the commit is d367d171faa515e5f4353d1164253bd1ad37aad4 Can we consider the issue closed?

[jmonticolo]

I just test it and it works perfectly.