Learner.load does not raise an exception if the provided filename was not found #74

Open basnijholt opened 5 years ago

(original issue on GitLab)

opened by Joseph Weston (@jbweston) at 2018-11-19T14:19:24.632Z

The default implementation is:

with suppress(FileNotFoundError, EOFError):
    data = load(fname, compress)
    self._set_data(data)

This means that if 'fname' does not exist this method silently fails. IMO it is the caller's responsibility to catch any errors.

originally posted by Bas Nijholt (@basnijholt) at 2018-11-19T15:24:43.030Z on GitLab

This is actually a really great way of using it.

This means that if 'fname' does not exist this method silently fails. IMO it is the caller's responsibility to catch any errors.

That doesn't happen, because this is always called.

originally posted by Joseph Weston (@jbweston) at 2018-11-19T17:12:48.672Z on GitLab

This is actually a really great way of using it.

I don't understand what this means.

If I call learner.load('doesnotexistlol') I would expect it to raise an exception and tell me that the file does not exist. Is it not confusing if your script just carries on with no data in the learner?

originally posted by Joseph Weston (@jbweston) at 2018-12-07T13:55:14.137Z on GitLab

So it seems that there is a difference between what we both consider as sane behaviour.

My understanding of the arguments is as follows

Raise an exception when loading from a data file that does not exist

This makes sense because most other APIs that deal with reading files raise exceptions when the files don't exist, e.g. open().

If you write code that says "load some data from file X" then your expectation is that there exists a file X with loadable data in it, and we should load this data. If any of these assumptions is false, an exception should be raised.

My opinion is that we should not make too many decisions for users. In this instance that means that we should err on the side of giving back more information than necessary (raising an exception), rather than too little.

Silently continuing and not loading any data

This makes sense because it means that the code used in the workflow of @basnijholt can be more streamlined.

Instead of

try:
    learner.load('data.txt')
except:
    pass

...

we can write

learner.load('data.txt')

...

originally posted by Joseph Weston (@jbweston) at 2018-12-07T13:56:30.174Z on GitLab

@Jorn @anton-akhmerov opinions

originally posted by Bas Nijholt (@basnijholt) at 2018-12-07T14:01:07.991Z on GitLab

The silencing is useful when running a BalancingLearner with some parameters combinations only and then the next time adding some parameter.

like (from the documentation)

def combo_fname(learner):
    val = learner.function.keywords  # because functools.partial
    fname = '__'.join([f'{k}_{v}.pickle' for k, v in val])
    return 'data_folder/' + fname

def f(x, a, b):
    return a * x**2 + b

combos = adaptive.utils.named_product(a=[1, 2], b=[1])
learners = [Learner1D(functools.partial(f, **combo), (-1, 1)) for combo in combos]
learner = BalancingLearner(learners)
runner = adaptive.Runner(learner)
learner.save(combo_fname)  # use 'load' in the same way

Then you change

combos = adaptive.utils.named_product(a=[1, 2], b=[1, 2, 3])

and a part of the data will be loaded.

basnijholt / adaptive