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

Added RSF writer and tests #301

Merged
merged 6 commits into from
Nov 27, 2023
Merged

Added RSF writer and tests #301

merged 6 commits into from
Nov 27, 2023

Conversation

aaronjgirard
Copy link
Collaborator

Description

Added a method to write out RSF files and included tests for the code.

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes.
  • documented the new feature with docstrings or appropriate doc page.
  • included a test. See testing guidelines.
  • your name has been added to the contributors page (docs/contributors.md).
  • added the "ready_for_review" tag once the PR is ready to be reviewed.

@aaronjgirard aaronjgirard added IO Work for reading/writing different formats ready_for_review PR is ready for review labels Nov 21, 2023
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (704d19f) 99.29% compared to head (cee50a9) 99.30%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #301   +/-   ##
=======================================
  Coverage   99.29%   99.30%           
=======================================
  Files          84       86    +2     
  Lines        6714     6771   +57     
=======================================
+ Hits         6667     6724   +57     
  Misses         47       47           
Flag Coverage Δ
unittests 99.30% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@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.

A couple small things then should be good to merge when you are happy with it.

dascore/io/rsf/core.py Outdated Show resolved Hide resolved
dascore/io/rsf/core.py Show resolved Hide resolved
dascore/io/rsf/__init__.py Outdated Show resolved Hide resolved
dascore/io/rsf/core.py Outdated Show resolved Hide resolved
tests/test_io/test_rsf/test_rsf.py Outdated Show resolved Hide resolved
tests/test_io/test_rsf/test_rsf.py Outdated Show resolved Hide resolved
@aaronjgirard
Copy link
Collaborator Author

aaronjgirard commented Nov 22, 2023 via email

@d-chambers
Copy link
Contributor

d-chambers commented Nov 22, 2023

Do you have an example test dataset with complex numbers, for example?

We can use patch.dft to generate one like this: complex_patch = random_patch.dft("time")

But I think we are not supporting that now? This line explicitly converts any data type to float 32. If this is what you want to do you can essentially remove lines 68-73 and just have file_formt = 'data_format="native_float"' which will remove the untested lines and make codecov happy.

@aaronjgirard
Copy link
Collaborator Author

aaronjgirard commented Nov 22, 2023

But I think we are not supporting that now? This line explicitly converts any data type to float 32. If this is what you want to do you can essentially remove lines 68-73 and just have file_formt = 'data_format="native_float"' which will remove the untested lines and make codecov happy.

The only problem then is in line 72-73, if someone ever tries to export a complex valued data set it cannot export because patch = spool[0] in line 56 cannot export and indeed fails when the following is added in the test:

complex_patch = random_patch.dft("time")
spool = dc.spool(complex_patch)

@d-chambers
Copy link
Contributor

The only problem then is in line 72-73, if someone ever tries to export a complex valued data set it cannot export because patch = spool[0] in line 56 cannot export and indeed fails when the following is added in the test:

This is a bug. I will open another issue.

The dtype conversion will probably fail if it is complex, but should work fine with ints (it will just cast the ints to floats, which is probably fine).

@d-chambers
Copy link
Contributor

see #303.

Once we get that fixed we can either explicitly support the complex type or just raise an error if the array dtype is complex. You can check it with np.issubdtype(dtype, np.complexfloating)

@aaronjgirard
Copy link
Collaborator Author

it looks like #304 fixed the complex values to spool issue, so I changed the RSF write and the test. However, now there another issue for writing integers to spool.

import dascore as dc

random_patch = dc.get_example_patch()
int_patch = random_patch.data.astype(np.int32)
spool = dc.spool(int_patch) # this raises error: "msg = 'Could not get spool from: {obj}" 

@d-chambers
Copy link
Contributor

I think that is actually the expected behavior. The problem is int_patch = random_patch.data.astype(np.int32) returns a numpy array (not a patch) with dtype int32, and dascore doesnt know how to make a spool from that.

Try this:

import numpy as np

import dascore as dc

int_patch = random_patch.new(data=np.ones_like(random_patch.data)) 

@aaronjgirard aaronjgirard merged commit 9d73d17 into master Nov 27, 2023
12 checks passed
@aaronjgirard aaronjgirard changed the title Added RF writer and tests Added RSF writer and tests Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO Work for reading/writing different formats ready_for_review PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants