Do not pass filter_parameters to the filehandler creation
#675
Conversation
Codecov Report
@@ Coverage Diff @@
## master #675 +/- ##
==========================================
+ Coverage 78.66% 78.69% +0.02%
==========================================
Files 138 138
Lines 20356 20390 +34
==========================================
+ Hits 16013 16045 +32
- Misses 4343 4345 +2
Continue to review full report at Codecov.
|
| @@ -709,6 +709,9 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None, | |||
| """ | |||
| reader_instances = {} | |||
| reader_kwargs = reader_kwargs or {} | |||
| reader_kwargs_without_filter = reader_kwargs.copy() | |||
| reader_kwargs_without_filter.pop('filter_parameters', None) | |||
There was a problem hiding this comment.
You could do:
filter_parameters = reader_kwargs.pop('filter_parameters', None)
and pass reader_kwargs and filter_parameters to the reader creation, but only reader_kwargs to the file handlers. Saves a copy, but not a big deal.
There was a problem hiding this comment.
ok, so, we need to decide, what are we rooting for ? filter_parameters as standalone parameters or part of reader_kwargs?
There was a problem hiding this comment.
I was just making a suggestion to clean up the code. It is the same interface on the reader and to this load_readers function. I'm saying you don't need to copy the dictionary if you pass filter_parameters explicitly:
filter_parameters = reader_kwargs.pop('filter_parameters', None)
reader = reader_cls(filter_parameters=filter_parameters, **reader_kwargs)
...
reader.create_filehandlers(**reader_kwargs)
There was a problem hiding this comment.
yes, agreed, but I'd like to take this opportunity to clarify things :) what is the "right" way ? This way we could maybe add a deprecation warning or something ?
There was a problem hiding this comment.
I think in the long-long run we remove the ability to filter things through load_readers (and through that the Scene.__init__.
In the short term the user should be specifying filter_parameters as a separate kwarg but the Scene.__init__ already has a deprecation warning for this and moves start_time/end_time/area to filter_parameters for the user. If you want to move filter_parameters outside of reader_kwargs and make it a separate/explicit kwarg I guess I'd be ok with that and maybe would prefer that.
There was a problem hiding this comment.
I think removing filter_parameters from the docstring may have been the opposite of what I suggested, but given that the long term decision is to probably remove all filtering from reader creation it doesn't really matter. This is an internal function right now anyway.
Also, given the need for this bug fix in master I don't want to hold this up any longer.
git diff origin/master -- "*py" | flake8 --diff