Closed dkrako closed 2 years ago
I now also added parameters for gamma, alpha and beta where it was used and documented all those newly added parameters.
Hey Daniel,
thank you for the PR!
The built-in Composites are the most commonly used set of rules. By Zennit's design of simplicity and customizability, I would rather like to keep them as simple as possible to encourage custom Composites using abstract Composites like LayerMapComposite instead when the use-case of changing them arises.
If you have not done so already, you may have a look at the User Guide on Composites, where examples are shown for defining a custom set of rules using LayerMapComposite
, SpecialFirstLayerMapComposite
and NameMapCompposite
.
Regarding AlphaBeta, it is not really common to choose anything except alpha=2, beta=1 and alpha=1, beta=0, so I think the alpha-beta parameters should be seen more as a static rule, thus I do not think this is a good addition.
Regarding Epsilon and Gamma, I think changing those may have actual use-cases, although I am not quite convinced yet given the customizability argument.
I know of one instance where the someone wanted to only change the gamma parameter in EpsilonGammaBox
.
Did the change of the epsilon parameter arise in your research? Maybe if this is common enough, it may be indeed a good idea to introduce the epsilon parameter into the __init__
of the Composites.
What are your thoughts and experiences as a user? Does it seem cumbersome to you to implement a custom composite only to change these parameters?
Thanks for the info!
I was playing around with the epsilon parameter for debugging purposes. I didn't want to copy this composite, which basically did exactly the same except of using the default epsilon parameter, so I changed it in the python package itself. I thought there maybe will be some other people who will want to change the epsilon/gamma parameter in this composite, so why not spare them the time when I have already done the "work".
From my point of view, the addition of epsilon/gamma doesn't do any harm in terms of complexity, as most people won't bother changing these default values. But I'm not the maintainer of the package, and I can move this to my own project repos. I haven't used the alpha beta rule so far, so I didn't know this. I just did it for the other rules too in a free minute.
Just leave me a message when you have decided what to do with this PR. There are also pylint and doc tests failing. I'm not completely sure why, but I'll check on this later after your decision.
Have a nice long weekend!
Looks great, thanks again!
I thought it would be nice to have the epsilon parameter available for the composites that include the epsilon rule.
Is it necessary to include tests for this simple addition? Code test coverage shouldn't change at all with this addition.