Closed pmelchior closed 7 years ago
@fred3m There are several aspects that I'd like to change here:
traceback
in its init that doesn't serve any purpose. Remove it.add_history
requires Z
and U
as well as step_f
and step_g
, which is overkill for PGM-like algorithms. I suggest to make Z,U,step_g optional with default None
.get_history
and get_errors
should be (self, key=None, j=0, m=0)
because key
is most important and 0 is the default for the latter two.I agree with points 1 and 2 and will update the code accordingly.
The reason I had to update the history before the updates are made is because I wanted to be able to duplicate the calculations, so that (for example) running prox_likelihoodA
on the variables in the current step in history would recreate A in the next step. If the history is updated after the variables are updated, then step_f
and step_g
are out of sync by one. So I didn't include the last iteration of the variables (since all matricesX[j]
in the last step are returned anyway) but I agree that it might be useful to have the final values of Z
and U
.
So I think it is better to leave this part of the code as is, but update the history once the loop has finished with the current values in the variables, this way everything is in sync. This will require using np.nan
for the final values of step_f
and step_g
, which should be ok.
I get your point about the key signatures. I wasn't thinking about the case with a single matrix (since I've never used that configuration) so I wanted the user to always specify which X[j]
matrix they wanted (so there is currently no default value for j
). But more generally I think that you're right, so I'll update the key signatures too.
Thanks for 1,2,4.
Re 3: You have a point, I didn't think of the steps. But the same problem applies to the errors, which are now calculated after the update but stored with the same index as the pre-update X,Z,U. And since the steps aren't changed until the go to the top of the main loop, you can safe all variables consistently if you do it after the updates. This makes the code also more straightforward to read and avoids missing the last update (I made that mistake). It does require that traceback is initialized before the algorithm runs.
The discussion above made me realize that there is a problem with the add_history
implementation. If it==0
, it will erase the previous contents. That's not ideal because the main loop will set it
to zero when it restarts with a smaller step_f
. The history would this be lost. I suggest that traceback is initialized with the first batch of X,Z,U,step_f,step_g and add_history
really only adds (as is implied by its name).
I think add_history
and add_errors
could be joined. They are now called at the same time and do exactly the same thing. The new init from 5. would apply to them as well.
Not only do I agree with 5 and 6, but I already started to make a more generic interface that combines both history and errors. It allows the user to specify arbitrary kwargs
so that when debugging in the future it will be easy to add additional variables to keep track of (and since the error values can easily be calculated from the other variables, only S and R are worth saving for each step anyway).
I'll let you know when it is pushed.
I'm not a great fan of the implicit definition of j=0
in __getitem__
, but I think it's fine for now.
Make it easier and more comprehensive to work with algorithm traceback