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.

Sign up for GitHub

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

Jump to bottom

Do not pass filter_parameters to the filehandler creation #675

Merged
merged 7 commits into from
Mar 26, 2019
Merged

Do not pass filter_parameters to the filehandler creation #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8664e35
Do not pass `filter_parameters` to the filehandler creation
mraspaud Mar 25, 2019
1247be0
Fix filter_parameters removal
mraspaud Mar 25, 2019
2f289d0
Test that filter_parameters isn't passed to the filehandlers
mraspaud Mar 26, 2019
407b916
Make sure the reader_kwargs passed to Scene() aren't modified in place
mraspaud Mar 26, 2019
ee877c4
Fix filter_parameters removal
mraspaud Mar 26, 2019
1026bbb
Remove deprecated documentation
mraspaud Mar 26, 2019
58e9d64
Fix filter_parameters removal test
mraspaud Mar 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions satpy/readers/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,8 +699,6 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None,
filenames (iterable or dict): A sequence of files that will be used to load data from. A ``dict`` object
should map reader names to a list of filenames for that reader.
reader (str or list): The name of the reader to use for loading the data or a list of names.
filter_parameters (dict): Specify loaded file filtering parameters.
Shortcut for `reader_kwargs['filter_parameters']`.
reader_kwargs (dict): Keyword arguments to pass to specific reader instances.
ppp_config_dir (str): The directory containing the configuration files for satpy.

Expand All @@ -709,6 +707,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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, so, we need to decide, what are we rooting for ? filter_parameters as standalone parameters or part of reader_kwargs?

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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.


if ppp_config_dir is None:
ppp_config_dir = get_environ_config_dir()

Expand Down Expand Up @@ -749,7 +750,7 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None,
if readers_files:
loadables = reader_instance.select_files_from_pathnames(readers_files)
if loadables:
reader_instance.create_filehandlers(loadables, fh_kwargs=reader_kwargs)
reader_instance.create_filehandlers(loadables, fh_kwargs=reader_kwargs_without_filter)
reader_instances[reader_instance.name] = reader_instance
remaining_filenames -= set(loadables)
if not remaining_filenames:
Expand Down
2 changes: 2 additions & 0 deletions satpy/scene.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ def __init__(self, filenames=None, reader=None, filter_parameters=None, reader_k
if filter_parameters:
if reader_kwargs is None:
reader_kwargs = {}
else:
reader_kwargs = reader_kwargs.copy()
reader_kwargs.setdefault('filter_parameters', {}).update(filter_parameters)

if filenames and isinstance(filenames, str):
Expand Down
50 changes: 50 additions & 0 deletions satpy/tests/test_scene.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,26 @@ def test_start_end_times(self):
self.assertEqual(scene.start_time, r.start_time)
self.assertEqual(scene.end_time, r.end_time)

def test_init_preserve_reader_kwargs(self):
import satpy.scene
from satpy.tests.utils import create_fake_reader
from datetime import datetime
with mock.patch('satpy.scene.Scene.create_reader_instances') as cri:
r = create_fake_reader('fake_reader',
start_time=datetime(2017, 1, 1, 0, 0, 0),
end_time=datetime(2017, 1, 1, 1, 0, 0),
)
cri.return_value = {'fake_reader': r}
reader_kwargs = {'calibration_type': 'gsics'}
scene = satpy.scene.Scene(filenames=['bla'],
base_dir='bli',
sensor='fake_sensor',
filter_parameters={'area': 'euron1'},
reader_kwargs=reader_kwargs)
self.assertIsNot(reader_kwargs, cri.call_args[1]['reader_kwargs'])
self.assertEqual(scene.start_time, r.start_time)
self.assertEqual(scene.end_time, r.end_time)

def test_init_alone(self):
from satpy.scene import Scene
from satpy.config import PACKAGE_CONFIG_PATH
Expand Down Expand Up @@ -168,6 +188,36 @@ def test_create_reader_instances_with_reader(self):
reader_kwargs=None,
)

def test_create_reader_instances_with_reader_kwargs(self):
import satpy.scene
from satpy.tests.utils import create_fake_reader
from datetime import datetime
filenames = ["1", "2", "3"]
reader_kwargs = {'calibration_type': 'gsics'}
filter_parameters = {'area': 'euron1'}
reader_kwargs2 = {'calibration_type': 'gsics', 'filter_parameters': filter_parameters}

with mock.patch('satpy.readers.load_reader') as lr_mock:
r = create_fake_reader('fake_reader',
start_time=datetime(2017, 1, 1, 0, 0, 0),
end_time=datetime(2017, 1, 1, 1, 0, 0),
)
lr_mock.return_value = r
r.select_path_from_pathnames.return_value = filenames
scene = satpy.scene.Scene(filenames=['bla'],
base_dir='bli',
sensor='fake_sensor',
filter_parameters={'area': 'euron1'},
reader_kwargs=reader_kwargs)
del scene
self.assertDictEqual(reader_kwargs, r.create_filehandlers.call_args[1]['fh_kwargs'])
scene = satpy.scene.Scene(filenames=['bla'],
base_dir='bli',
sensor='fake_sensor',
reader_kwargs=reader_kwargs2)
self.assertDictEqual(reader_kwargs, r.create_filehandlers.call_args[1]['fh_kwargs'])
del scene

def test_iter(self):
from satpy import Scene
from xarray import DataArray
Expand Down