Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

read/write support for csv, eventtxt and csz #3285

Merged
merged 26 commits into from
Mar 30, 2023
Merged

read/write support for csv, eventtxt and csz #3285

merged 26 commits into from
Mar 30, 2023

Conversation

trichter
Copy link
Member

@trichter trichter commented Mar 24, 2023

What does this PR do?

Id adds read and write support for csv, eventtxt and csz. csz is a custom format and basically a collection of csv files bundled in a zip file (similar to numpys npz format) for io of a catalog with picks.

Why was it initiated? Any relevant Issues?

Fixes #1467

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • While the PR is still work-in-progress, the no_ci label can be added to skip CI builds
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just add the build_docs tag to this PR.
    Docs will be served at docs.obspy.org/pr/{branch_name} (do not use master branch).
    Please post a link to the relevant piece of documentation.
  • If all tests including network modules (e.g. clients.fdsn) should be tested for the PR,
    just add the test_network tag to this PR.
  • All tests still pass.
  • Any new features or fixed regressions are covered via new tests.
  • Any new or changed features are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .
  • If the changes affect any plotting functions you have checked that the plots
    from all the CI builds look correct. Add the "upload_plots" tag so that plotting
    outputs are attached as artifacts.
  • New modules, add the module to CODEOWNERS with your github handle
  • Add the yellow ready for review label when you are ready for the PR to be reviewed.

@trichter trichter added the build_docs Docs will be automatically built and deployed in github actions on pushes to the PR label Mar 24, 2023
@trichter trichter added the .io issues generally related to our read/write plugins label Mar 24, 2023
@trichter trichter added this to the 1.5.0 milestone Mar 24, 2023
@trichter
Copy link
Member Author

Doc link. Some functions are mising its doc string.

@trichter
Copy link
Member Author

Do we need an __init__.py file in the tests directory for the tests to be discovered by pytest? Why?

@trichter trichter added the ready for review PRs that are ready to be reviewed to get marked ready to merge label Mar 24, 2023
@d-chambers
Copy link
Member

Do we need an init.py file in the tests directory for the tests to be discovered by pytest? Why?

Do we have another 'test_csv.py' anywhere? If so, that explains it. Pytest's test importer is a bit wonky. Maybe try giving your test file a more unique name?

@megies
Copy link
Member

megies commented Mar 24, 2023

Do we have another 'test_csv.py' anywhere?

Doesn't look like it.

megies
megies previously approved these changes Mar 24, 2023
Copy link
Member

@megies megies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still needs changelog entry, otherwise looks good to me 🚀

@megies
Copy link
Member

megies commented Mar 24, 2023

Ah and needs to be added here: misc/docs/source/packages/index.rst

Copy link
Member

@d-chambers d-chambers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good addition, I just left a few suggestions.

obspy/io/csv/tests/test_csv.py Outdated Show resolved Hide resolved
Comment on lines +108 to +111
try:
import zlib # noqa: F401
except ImportError:
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider suppress from context lib here to make it more concise and clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not change anything here for now

obspy/io/csv/tests/test_csv.py Outdated Show resolved Hide resolved
obspy/io/csv/core.py Outdated Show resolved Hide resolved
obspy/io/csv/core.py Outdated Show resolved Hide resolved
obspy/io/csv/core.py Outdated Show resolved Hide resolved
Comment on lines +164 to +232
with _open(fname) as f:
for _ in range(skipheader):
f.readline()
if names is not None:
kwargs.setdefault('fieldnames', _names_sequence(names))
reader = csv.DictReader(f, **kwargs)
for row in reader:
if 'time' in row:
time = UTC(row['time'])
else:
time = UTC(
'{year}-{mon}-{day} {hour}:{minu}:{sec}'.format(**row))
try:
if 'depm' in row:
dep = float(row['depm'])
else:
dep = float(row['dep']) * 1000
if math.isnan(dep):
raise
except Exception:
dep = None
author = _string(row, 'author')
contrib = _string(row, 'contrib')
if author is not None or contrib is not None:
info = evmod.CreationInfo(author=author, agency_id=contrib)
else:
info = None
origin = evmod.Origin(
time=time,
latitude=row['lat'],
longitude=row['lon'],
depth=dep,
creation_info=info
)
try:
# add zero to eliminate negative zeros in magnitudes
mag = float(row['mag']) + 0
if math.isnan(mag):
raise
except Exception:
magnitudes = []
else:
try:
magtype = row['magtype']
if magtype.lower() in ('', 'none', 'null', 'nan'):
raise
except Exception:
magtype = default.get('magtype')
magauthor = _string(row, 'magauthor')
info = (evmod.CreationInfo(author=magauthor) if magauthor
else None)
magnitudes = [evmod.Magnitude(
mag=mag, magnitude_type=magtype, creation_info=info)]
try:
id_ = evmod.ResourceIdentifier(row['id'].strip())
except Exception:
id_ = None
region = _string(row, 'region')
descs = ([evmod.EventDescription(region, 'region name')]
if region else [])
event = evmod.Event(
magnitudes=magnitudes,
origins=[origin],
resource_id=id_,
event_descriptions=descs
)
events.append(event)
if format_check:
return True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly optional, but this is a bit long. Maybe can be refactored into separate helper functions for reading each part?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's postpone that

Comment on lines +272 to +279
try:
author = origin.creation_info.author
except AttributeError:
author = ''
try:
contrib = origin.creation_info.agency_id
except AttributeError:
contrib = ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These cant be re-phrased as getattr with default values because both creation_info and creation_info's sub attribute might be missing right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly

obspy/io/csv/core.py Outdated Show resolved Hide resolved
obspy/io/csv/core.py Show resolved Hide resolved
@megies
Copy link
Member

megies commented Mar 28, 2023

Thanks for the thorough review @d-chambers, definitely some very valid points there 😃

@megies megies dismissed their stale review March 28, 2023 08:23

better review showing need for some changes

Copy link
Member

@megies megies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I collapsed everything that was addressed and made linter happy. Looks like the rest is optional stuff that can be left as is?

@d-chambers
Copy link
Member

👍 looks good to me.

@megies megies merged commit c1ce508 into master Mar 30, 2023
@megies megies deleted the csv branch March 30, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build_docs Docs will be automatically built and deployed in github actions on pushes to the PR .io issues generally related to our read/write plugins ready for review PRs that are ready to be reviewed to get marked ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read/write support for eventtxt format
3 participants