search

microsoft / vision-explanation-methods

Methods for creating saliency maps for computer vision models.
https://vision-explanation-methods.readthedocs.io/en/latest/
MIT License
39 stars 12 forks source link

API design feedback - get_drise_saliency_map #32

Open ksachdeva opened 1 year ago

ksachdeva commented 1 year ago

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 1 year ago

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.

ejones18 commented 1 month ago

Hi both, see #52 - made the suggested changes