eqcorrscan / EQcorrscan

Earthquake detection and analysis in Python.
https://eqcorrscan.readthedocs.io/en/latest/
Other
166 stars 86 forks source link

Conflit lag_calc with input template names #346

Closed ikahbasi closed 5 years ago

ikahbasi commented 5 years ago

Is your feature request related to a problem? Please describe. I used event's origin time as input of lag_calc's template_names in 0.3.3 version. But when i ran my code in 0.4.0v, there was a problem with it. I had to replace all ":" and "." with "", and "TZ" with "tz". It makes some bad view in naming of output plots. I found It happens because of regex lines (78 to 82) in template,py.

Describe the solution you'd like It isn't a hard problem certainly. I like to use good format of origin-time, but this regex is necessary somewhere certainly. Then is it possible to change this regex to accept some more patterns? For example something include '-' instead of ':' or etc.

calum-chamberlain commented 5 years ago

The template name regex hasn't changed from version 0.3.3. I just downloaded the 0.3.3 source code to check and the same lines are there:

        name_regex = re.compile(r"^[a-z_0-9]+$")
        if name is not None and not re.match(name_regex, name):
            raise ValueError("Invalid name: '%s' - Must satisfy the regex "
                             "'%s'." % (name, name_regex.pattern))

The reason for this is:

  1. Colons cause issues in windows path-names (see #344 for a current example) - to maintain platform independence I chose to not allow colons in template names (internal writing routines write templates using their name);
  2. Dots are usually saved for file extensions.
  3. The upper/lower case things is again because some filesystems do not distinguish between upper and lower cases.

I don't know of a reason not to include dashes in the regex, and a PR with that minor change would be good! :+1:

Just needs to change to:

name_regex = re.compile(r"^[-a-z_0-9]+$")
ikahbasi commented 5 years ago

Colons cause issues in windows path-names.... I use ubuntu and there is exist there.

I don't know of a reason not to include dashes in the regex, and a PR with that minor change would be good! 👍

I will make this change tomorrow. I'm waiting for last pull request's status to make a new PR for these changes.

calum-chamberlain commented 5 years ago

I will add you as a contributor, then you can create branches here rather than keeping everything in your fork.

ikahbasi commented 5 years ago

I will add you as a contributor, then you can create branches here rather than keeping everything in your fork.

Thank you for this :)

calum-chamberlain commented 5 years ago

Closed by #348