Added save_dir argument to save_detector

[ascillitoe]

Fix for #348.

An optional save_dir argument is used, instead of detector_name used in fetch_detector, so as to not confuse the detector_namevariable already used insave_detector. This also matches the syntax used in save_tf_model` etc.

If save_dir=None then the detector_name (from metadata) is used for save_dir instead.

[jklaise]

Looking back on your comment I realize I misread it and thought your suggestion was to change the behaviour of fetch_detector. I think the behaviour of save_detector is already correct and should be kept as simple as possible without extra arguments.

[ascillitoe]

@jklaise perhaps this is the most straightforward fix? There is no confusing logic for the user to contend with.

We would have to modify all the notebooks though e.g. delete the filepath = os.path.join(filepath, detector_name) line in cd_distillation_cifar10.ipynb.

[jklaise]

This is probably what the user would expect to happen so I agree.

Wrt notebooks, should we modify the saved paths to be ./models/detector_name so the current behaviour is unchanged (i.e. running multiple notebooks saves all detectors in their own subdirectory so they can be retrieved later).

[ascillitoe]

That's a much nicer solution with the notebooks, I'll do that 👍🏻

[ascillitoe]

I've had a go at moving the os.path.join(filepath,detector_name) outside the if load_pretrained: block, so filepath should be consistent for fetch and save routes.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[ascillitoe]

@jklaise I'm just going to check the docs examples one more time to check for any more issues like the two you found.

[ascillitoe]

@jklaise I'm just going to check the docs examples one more time to check for any more issues like the two you found.

Might still have missed something, but have gone through all the notebooks with changes and didn't spot any other issues like the above.