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.

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

Padim script #15

Closed
wants to merge 2 commits into from
Closed

Padim script #15

wants to merge 2 commits into from

Conversation

VictorOlof
Copy link
Contributor

No description provided.

@antonemanuel
Copy link
Contributor

antonemanuel commented Oct 22, 2021

The idea of the function was to test different sets of parameters. E.g. different datasets. So the function should take some list or interval of values/paths/data and then try the different parameters given. Now it seems like this is not the case. You can start with just making the function being able to differ just one parameter for simplicity.

However maybe it is nice to have the function you made for one of these iterations. And have a wrapper function for looping over the different sets of parameters.

Other fixes:

  • The file should not be under notebooks. Only files under the directory anodet is part of the package.
  • You don't need to save the means and inverse covariances, and then load them again. So remove the saving and loading of the parameters.
  • Instead of visualising the results with images, I think it's easier to start with just printing the values of ROC-AUC score, recall, and precision (look in the tests_example notebook).
  • It seems like the function is being called in the end of the file. This should not be there.
  • Name the functions something more specific than run and visual, so people get a hint of what they do.
  • Add docstrings to the functions.
  • There should not be any defaults on the data paths. It is very unlikely that other people will have the data saved in the same directory structure as you.
  • Add an example of how to use the function in the tests_example notebook.

@antonemanuel antonemanuel linked an issue Oct 22, 2021 that may be closed by this pull request
@antonemanuel antonemanuel self-requested a review October 22, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test script
2 participants