Skip to main content
Commonmark migration
Source Link

##IndicalDistributions

IndicalDistributions

##BoundaryConditions

BoundaryConditions

##CartesianRandomWalker

CartesianRandomWalker

##IndicalDistributions

##BoundaryConditions

##CartesianRandomWalker

IndicalDistributions

BoundaryConditions

CartesianRandomWalker

Source Link

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.