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

Small change so that read_RADOLAN_composite also accepts a file handle #114

Merged
merged 4 commits into from Oct 25, 2016

Conversation

Projects
None yet
4 participants
@cchwala
Contributor

cchwala commented Oct 24, 2016

Not sure if this could be useful for others.

But since I want to work with the different archived RADOLAN files more easily (current bin.gz files and historic tar.gz files) I added the possibility to pass a file handle to read_RADOLAN_composite. This makes it possible to untar and unzip the historic RADOLAN files on the fly and avoids storing uncompressed data or creating temporary files from the tar.gz files. I tried the approach with the temporary files, but this small change to read_RADOLAN_composite is much more elegant.

I can however understand that this is just a edge-case and of no use to most other users so that a merge does not make sense.

@kmuehlbauer

@cchwala

It makes sense, to have filehandle or filename in this case. I added a suggestion on how to remove the typecheck.

@@ -846,7 +846,9 @@ def read_RADOLAN_composite(fname, missing=-9999, loaddata=True):
NODATA = missing
mask = 0xFFF # max value integer
f = get_radolan_filehandle(fname)

This comment has been minimized.

@kmuehlbauer

kmuehlbauer Oct 24, 2016

Member

We can try to prevent the explicit type-check like this:

try:
    header = read_radolan_header(f)
except AttributeError:
    f = get_radolan_filehandle(f)
    header = read_radolan_header(f)
@kmuehlbauer

kmuehlbauer Oct 24, 2016

Member

We can try to prevent the explicit type-check like this:

try:
    header = read_radolan_header(f)
except AttributeError:
    f = get_radolan_filehandle(f)
    header = read_radolan_header(f)

This comment has been minimized.

@cchwala

cchwala Oct 24, 2016

Contributor

Okay. Should I update my pull request, or will you add your suggested solution?

@cchwala

cchwala Oct 24, 2016

Contributor

Okay. Should I update my pull request, or will you add your suggested solution?

@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Oct 24, 2016

Member

@heistermann Can we handle it this way? This won't break the API, but extend it.

Member

kmuehlbauer commented Oct 24, 2016

@heistermann Can we handle it this way? This won't break the API, but extend it.

@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Oct 24, 2016

Member

@cchwala

Yes, please update your PR. And thanks for this useful addition. Now I do not have to extract the monthly files anymore...

Member

kmuehlbauer commented Oct 24, 2016

@cchwala

Yes, please update your PR. And thanks for this useful addition. Now I do not have to extract the monthly files anymore...

@cchwala

This comment has been minimized.

Show comment
Hide comment
@cchwala

cchwala Oct 25, 2016

Contributor

On my system, and also on travis-ci, test_radolan_coords fails. I do not see how this is related, though.

Contributor

cchwala commented Oct 25, 2016

On my system, and also on travis-ci, test_radolan_coords fails. I do not see how this is related, though.

@heistermann

This comment has been minimized.

Show comment
Hide comment
@heistermann

heistermann Oct 25, 2016

Collaborator

I agree - I also don't see how your PR would break the test. Appears to be some accuracy issue... a quick fix would be to replace the assertEqual by assertAlmostEqual in the failing test_radolan_coords. Any thoughts about this, @kmuehlbauer ?

Collaborator

heistermann commented Oct 25, 2016

I agree - I also don't see how your PR would break the test. Appears to be some accuracy issue... a quick fix would be to replace the assertEqual by assertAlmostEqual in the failing test_radolan_coords. Any thoughts about this, @kmuehlbauer ?

@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Oct 25, 2016

Member

Any thoughts about this, @kmuehlbauer ?

See #115

Member

kmuehlbauer commented Oct 25, 2016

Any thoughts about this, @kmuehlbauer ?

See #115

@heistermann

This comment has been minimized.

Show comment
Hide comment
@heistermann

heistermann Oct 25, 2016

Collaborator

@cchwala Could you include this change into your PR so we can pull it in?

Collaborator

heistermann commented Oct 25, 2016

@cchwala Could you include this change into your PR so we can pull it in?

@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Oct 25, 2016

Member

@cchwala @heistermann

The test should look something like this:

x, y = georef.get_radolan_coords(7.0, 53.0)
self.assertAlmostEqual(x, -208.15159184860158)
self.assertAlmostEqual(y, -3971.7689758313813)
x, y = georef.get_radolan_coords(7.0, 53.0, trig=True)
self.assertEqual(x, -208.15159184860175)
self.assertEqual(y, -3971.7689758313832)

Maybe a push with these additions will also retrigger the CI. Not sure why travis-ci is still waiting for status. Maybe some github-api issue.

Member

kmuehlbauer commented Oct 25, 2016

@cchwala @heistermann

The test should look something like this:

x, y = georef.get_radolan_coords(7.0, 53.0)
self.assertAlmostEqual(x, -208.15159184860158)
self.assertAlmostEqual(y, -3971.7689758313813)
x, y = georef.get_radolan_coords(7.0, 53.0, trig=True)
self.assertEqual(x, -208.15159184860175)
self.assertEqual(y, -3971.7689758313832)

Maybe a push with these additions will also retrigger the CI. Not sure why travis-ci is still waiting for status. Maybe some github-api issue.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 25, 2016

Coverage Status

Coverage increased (+0.01%) to 82.124% when pulling c7c6360 on cchwala:read_radolan_from_file_handle into 6d54020 on wradlib:master.

coveralls commented Oct 25, 2016

Coverage Status

Coverage increased (+0.01%) to 82.124% when pulling c7c6360 on cchwala:read_radolan_from_file_handle into 6d54020 on wradlib:master.

@kmuehlbauer kmuehlbauer merged commit 50fc912 into wradlib:master Oct 25, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 82.124%
Details
@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Oct 25, 2016

Member

@cchwala

Thanks for adding this and working through the precision issue.

Member

kmuehlbauer commented Oct 25, 2016

@cchwala

Thanks for adding this and working through the precision issue.

@cchwala cchwala deleted the cchwala:read_radolan_from_file_handle branch Oct 27, 2016

@kmuehlbauer kmuehlbauer added this to the Release 0.10.0 milestone Mar 23, 2017

kmuehlbauer added a commit that referenced this pull request Feb 28, 2018

Merge pull request #114 from cchwala/read_radolan_from_file_handle
Small change so that read_RADOLAN_composite also accepts a file handle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment