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

ENH: added option to read rainbow files from file handle #140

Merged
merged 9 commits into from Apr 8, 2017

Conversation

Projects
None yet
2 participants
@cchwala
Contributor

cchwala commented Mar 30, 2017

As discussed, here, this is now the PR to make it possible to read Rainbow files from a file handle, e.g. when reading from a zipped archive.

I assumed that the low level functions do not need the apichange_kwarg since nobody ever uses them directly and they only get called by read_Rainbow. It would be hard (or impossible without writing to a temp file) to transform the fid file handles into the now deprecated filename strings because in many cases (or at least in my application) there is no path to the files which e.g. may belong to a zipped archive.

After rebasing on master I cannot run the tests. They fail with "The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec().". Hence, I hope that the CI runs will complete.

cchwala added some commits Mar 21, 2017

FIX: Updated rainbow low level functions
* now IOErrors are raised as required by the tests instead of AttributeErrors from, e.g. `fid.read()` if `fid` is a string
* removed the unnecessary `fid.close()`

cchwala added a commit to cchwala/rainscanner that referenced this pull request Mar 30, 2017

@cchwala

This comment has been minimized.

Show comment
Hide comment
@cchwala

cchwala Mar 30, 2017

Contributor

Sorry for the iterative fixing based on CI errors. As stated above, tests do currently not run locally on my machine. Feel free to squash the commits. The first one is labled wrongly anyway. "WIP" should now maybe be "ENH", when it's finally working...

Contributor

cchwala commented Mar 30, 2017

Sorry for the iterative fixing based on CI errors. As stated above, tests do currently not run locally on my machine. Feel free to squash the commits. The first one is labled wrongly anyway. "WIP" should now maybe be "ENH", when it's finally working...

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Mar 30, 2017

Codecov Report

Merging #140 into master will decrease coverage by 0.03%.
The diff coverage is 90.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
- Coverage   82.69%   82.65%   -0.04%     
==========================================
  Files          18       18              
  Lines        4467     4475       +8     
==========================================
+ Hits         3694     3699       +5     
- Misses        773      776       +3
Impacted Files Coverage Δ
wradlib/io.py 89.52% <90.24%> (-0.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f852563...e02b349. Read the comment docs.

codecov bot commented Mar 30, 2017

Codecov Report

Merging #140 into master will decrease coverage by 0.03%.
The diff coverage is 90.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
- Coverage   82.69%   82.65%   -0.04%     
==========================================
  Files          18       18              
  Lines        4467     4475       +8     
==========================================
+ Hits         3694     3699       +5     
- Misses        773      776       +3
Impacted Files Coverage Δ
wradlib/io.py 89.52% <90.24%> (-0.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f852563...e02b349. Read the comment docs.

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

@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Mar 31, 2017

Member

After rebasing on master I cannot run the tests. They fail with "The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec().". Hence, I hope that the CI runs will complete.

@cchwala Are you on OSX? On my linux box everything runs fine, when I start the tests on the console with python testrunner.py -a.
The changes were introduced with #139. We were forced to run testsuites in subprocesses to overcome the overall memory limit of 3GB on travis-ci.

Member

kmuehlbauer commented Mar 31, 2017

After rebasing on master I cannot run the tests. They fail with "The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec().". Hence, I hope that the CI runs will complete.

@cchwala Are you on OSX? On my linux box everything runs fine, when I start the tests on the console with python testrunner.py -a.
The changes were introduced with #139. We were forced to run testsuites in subprocesses to overcome the overall memory limit of 3GB on travis-ci.

@cchwala

This comment has been minimized.

Show comment
Hide comment
@cchwala

cchwala Mar 31, 2017

Contributor

@kmuehlbauer Yes OSX. I will try on a Linux machine. Should I open an issue concerning the tests on OSX?

Contributor

cchwala commented Mar 31, 2017

@kmuehlbauer Yes OSX. I will try on a Linux machine. Should I open an issue concerning the tests on OSX?

@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Mar 31, 2017

Member

@cchwala This is a problem obviously with OSX (and maybe with windows). I found this: http://stackoverflow.com/questions/16254191/python-rpy2-and-matplotlib-conflict-when-using-multiprocessing

But you can try xvfb to make the tests headless. See .travis.yml.

You can open an issue, and we can reference everything there.

Member

kmuehlbauer commented Mar 31, 2017

@cchwala This is a problem obviously with OSX (and maybe with windows). I found this: http://stackoverflow.com/questions/16254191/python-rpy2-and-matplotlib-conflict-when-using-multiprocessing

But you can try xvfb to make the tests headless. See .travis.yml.

You can open an issue, and we can reference everything there.

@kmuehlbauer kmuehlbauer referenced this pull request Mar 31, 2017

Closed

Release 0.10.0 #143

3 of 3 tasks complete

@kmuehlbauer kmuehlbauer self-requested a review Mar 31, 2017

@kmuehlbauer

The apichange_kwarg decorator works only for kwargs. We have common args here. So applying this decorator won't work anyway.

@heistermann Do we need an apichange_args decorator? How do we handle this?

Show outdated Hide outdated wradlib/io.py
@@ -1541,15 +1541,15 @@ def get_RB_blob_from_string(datastring, blobdict):
return data
def get_RB_blob_from_file(filename, blobdict):
def get_RB_blob_from_file(fid, blobdict):

This comment has been minimized.

@kmuehlbauer

kmuehlbauer Mar 31, 2017

Member

This is meant to be a convenience function. It is not used by any of the other rainbow low level functions.

It was designed to quickly sweep through a bunch of files gathering a specific known blob. It would be good to have both filename and fid within this function.

@kmuehlbauer

kmuehlbauer Mar 31, 2017

Member

This is meant to be a convenience function. It is not used by any of the other rainbow low level functions.

It was designed to quickly sweep through a bunch of files gathering a specific known blob. It would be good to have both filename and fid within this function.

Show outdated Hide outdated wradlib/io.py
xmltodict = util.import_optional('xmltodict')
return xmltodict.parse(header)
def read_Rainbow(filename, loaddata=True):
""""Reads Rainbow files files according to their structure
@util.apichange_kwarg('0.9.2', par='filename', typ=str, expar='f')

This comment has been minimized.

@kmuehlbauer

kmuehlbauer Mar 31, 2017

Member

Because this doesn't break the API (but extend it) we can remove the decorator here.
Please add a versionchanged:

    .. versionchanged 0.10.0
       Added reading from file handles.

in the docstring.

@kmuehlbauer

kmuehlbauer Mar 31, 2017

Member

Because this doesn't break the API (but extend it) we can remove the decorator here.
Please add a versionchanged:

    .. versionchanged 0.10.0
       Added reading from file handles.

in the docstring.

This comment has been minimized.

@cchwala

cchwala Apr 7, 2017

Contributor

Removed the decorator according to your request. But isn't the decorator also meant to resolve the issue where people have read_Rainbow(filename='my_file.azi') in their code?

@cchwala

cchwala Apr 7, 2017

Contributor

Removed the decorator according to your request. But isn't the decorator also meant to resolve the issue where people have read_Rainbow(filename='my_file.azi') in their code?

This comment has been minimized.

@kmuehlbauer

kmuehlbauer Apr 7, 2017

Member

filename is no kwarg, so it isn't catched by this decorator. We would need some decorator for args.

@kmuehlbauer

kmuehlbauer Apr 7, 2017

Member

filename is no kwarg, so it isn't catched by this decorator. We would need some decorator for args.

@cchwala

This comment has been minimized.

Show comment
Hide comment
@cchwala

cchwala Apr 4, 2017

Contributor

@kmuehlbauer Sorry, I am very busy this week. Would it suffice to finish this PR next monday?

Contributor

cchwala commented Apr 4, 2017

@kmuehlbauer Sorry, I am very busy this week. Would it suffice to finish this PR next monday?

@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Apr 4, 2017

Member

@cchwala I would really like to roll out wradlib release 0.10.0. on next monday. I could pull your additions from your fork and add my suggested changes. Are you OK with this way of working?

Member

kmuehlbauer commented Apr 4, 2017

@cchwala I would really like to roll out wradlib release 0.10.0. on next monday. I could pull your additions from your fork and add my suggested changes. Are you OK with this way of working?

@cchwala

This comment has been minimized.

Show comment
Hide comment
@cchwala

cchwala Apr 4, 2017

Contributor

@kmuehlbauer Okay, I will do it till Friday evening/night. It does not look like too much work anyway and since I started this I also want to bring it to an end ;-).

Contributor

cchwala commented Apr 4, 2017

@kmuehlbauer Okay, I will do it till Friday evening/night. It does not look like too much work anyway and since I started this I also want to bring it to an end ;-).

@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Apr 5, 2017

Member

@cchwala This would fit.

@heistermann Regarding the apichange. Since we do not have something for *args we do not have much options. From the workflow it seems unnecessary to open/close the file multiple times. So I tend to silently break the API for the low-level functions (leave the get_RB_blob_from_file handle both filename and fid). Adding .. versionchanged:: 0.10.0 in all changed functions should suffice.

Member

kmuehlbauer commented Apr 5, 2017

@cchwala This would fit.

@heistermann Regarding the apichange. Since we do not have something for *args we do not have much options. From the workflow it seems unnecessary to open/close the file multiple times. So I tend to silently break the API for the low-level functions (leave the get_RB_blob_from_file handle both filename and fid). Adding .. versionchanged:: 0.10.0 in all changed functions should suffice.

@kmuehlbauer kmuehlbauer referenced this pull request Apr 7, 2017

Merged

Release 0.10.0 #144

@cchwala

This comment has been minimized.

Show comment
Hide comment
@cchwala

cchwala Apr 7, 2017

Contributor

@kmuehlbauer #137 produces a diff for io.py that is the whole file content. I have a hard time to solve the merge conflict since its 2000 lines of code vs 2000 lines of code. Right now I am assuming that you only changed things concerning io.write_raster_dataset within io.py and will manually resolve the conflicts. Any additional info from your side?

Contributor

cchwala commented Apr 7, 2017

@kmuehlbauer #137 produces a diff for io.py that is the whole file content. I have a hard time to solve the merge conflict since its 2000 lines of code vs 2000 lines of code. Right now I am assuming that you only changed things concerning io.write_raster_dataset within io.py and will manually resolve the conflicts. Any additional info from your side?

@cchwala

This comment has been minimized.

Show comment
Hide comment
@cchwala

cchwala Apr 7, 2017

Contributor

@kmuehlbauer Merge done. Test should complete, at least they now do on my Mac. Now on to the requested changes.

Contributor

cchwala commented Apr 7, 2017

@kmuehlbauer Merge done. Test should complete, at least they now do on my Mac. Now on to the requested changes.

@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Apr 7, 2017

Member

@cchwala Sorry for the inconvenience with the io.py. I remember having issues with the line endings. That's probably the cause for the large difference.

Member

kmuehlbauer commented Apr 7, 2017

@cchwala Sorry for the inconvenience with the io.py. I remember having issues with the line endings. That's probably the cause for the large difference.

@kmuehlbauer kmuehlbauer changed the title from ENH: added option to read rainbow files from file handel to ENH: added option to read rainbow files from file handle Apr 8, 2017

@kmuehlbauer kmuehlbauer merged commit ac39298 into wradlib:master Apr 8, 2017

3 checks passed

codecov/patch 90.24% of diff hit (target 82.69%)
Details
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +7.54% compared to f852563
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmuehlbauer

This comment has been minimized.

Show comment
Hide comment
@kmuehlbauer

kmuehlbauer Apr 8, 2017

Member

@cchwala Thanks for your contribution! Hoping to see more from your side 👍.

Member

kmuehlbauer commented Apr 8, 2017

@cchwala Thanks for your contribution! Hoping to see more from your side 👍.

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

ENH: added option to read rainbow files from file handel (#140)
ENH: added option to read rainbow files from file handle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment