As a first comment, instead of specifying your types in the comments of each method, you can use the typing module for Python 3.5 and above https://docs.python.org/3/library/typing.html Also, I think the comments on variables should also describe what they represent.
Then, if I were you, I wouldn't try creating a very generic code, it usually leads to unnecessary over-engineering and more complex code to maintain. If you need more things, then you'll adapt your code to it as soon as you need. You usually first make it work with your new needs anyhow and then try to refactor it to make it better.
However, I'll try to give my point of view of how I'd change your code so it can be adapted more easily to what you said about multiple distributions.
##IndicalDistributions
The get_normal_indices
and the get_binomial_indices
functions don't really belong to this class. IndicalDistributions
should not know about all the possible distributions there can be. And you should definitely not store a dictionary with all of them in here, it leads to very messy and hard to maintain code.
- I think the easiest way to fix this, is to have separate implementations for each kind of distribution and choose the class when needed in your code, so you would end up removing
IndicalDistributions
and having something like NormalIndicalDistribution
and BinomialIndicalDistribution
classes separately. When you'll have to create BinomialIndicalDistribution
, you'll see what are the common parts of NormalIndicalDistribution
and BinomialIndicalDistribution
, and create some kind of abstraction to do the common stuff (maybe strategy or template method pattern).
If you want to specify strings to choose the correct distribution class like you do in dispatch_indices
, you can just have a function create_indical_distribution(distribution, **kwargs)
that's just a bunch of ifs that return the correct object constructed. This is usually called a factory method.
But again, for now just create one NormalIndicalDistribution
class and then you'll see what happens when you need the binomial one.
##BoundaryConditions
You said nothing about having multiple edge types apart from these 2, so I would not touch this and if I would, the approach would be similar to the class before.
##CartesianRandomWalker
self.base
is a reserved name for the Python language, name it something else, it can give you very weird bugs.
In view
, I think it'd be cleaner if you'd do something like
if self.ndim not in (1, 2, 3):
raise ValueError(f"invalid ndim: {self.ndim}; can only view 1 <= ndim <= 3")
if self.ndim in (1, 2):
raise ValueError("not yet implemented")
# Your implementation goes here
However, I think the implementation for each dimension should go to a separate class and maybe have some common utilities for each dimension, it'd be much easier to maintain. Similar approach to what I explained before.
Usually, instantiating classes in the middle of other classes is not a very good idea, the objects should be passed in __init__
(I mean IndicalDistributions
and BoundaryConditions
). They should be constructed before and passed to CartesianRandomWalker
, this will let you use more kinds of indical distributions and edge types, since you just pass whichever you want in the construction and that's it.
For IndicalDistributions
, it should be no problem, just remove the distribution string and pass the correct object when constructing.
However, the BoundaryConditions
object depends on parameters that you can only know in apply_boundary_conditions
. This is a bit tricky. Honestly, since you didn't say anything about having more than these two edge types, I'd leave as is, otherwise, look up Builder Pattern.