microsoft/vision-explanation-methods

API design feedback - get_drise_saliency_map

ksachdeva opened this issue · 2 comments

ksachdeva commented

Hi,

I believe that the api get_drise_saliency_map can be improved. Here are some suggestions -

  1. It is a good idea to move Optional arguments after the required arguments. For e.g. model is an optional argument but it is specifeid before numclasses (a required argument).

  2. Majority of the arguments of this api are not following snake_casing. Only model and max_figures are named as per the convention.

  3. It would be a good idea to define a NamedTuple and use it for the return values. It helps with readability and intellisense.

  4. If model is None then you use FastRCNN model underneath that has 87 classes. However you do require the user to pass numclasses. You can either make numclasses optional with default value of 87 or ignore it if model is None.

imatiach-msft commented

Thank you for the great suggestions! I agree with all of these API improvements. I don't have bandwidth to make these changes just now but I think we will try to make them in some near term future. If you wish to send a PR to make these changes sooner please feel free to do so - contributions are welcome.