Line-by-line comments for main classes

[wapu]

Properly comment ReversibleGraphNet and Node, it's a recursive nightmare and every time we want to change something we have to find out from scratch how it works...

[psteinb]

Might be worth considering to put it in a test harness and use this as documentation for future reference.

[wapu]

Where can I read up on that?

I am really going for comments on each line of code here, because what happens inside the class functions and how all the members interact can be hard to understand from the code alone.

[psteinb]

A test harness is for me a concept. I learned about it in Michael C. Feathers book "Working effectively with legacy code" (some sample pages http://ptgmedia.pearsoncmg.com/images/9780131177055/samplepages/0131177052.pdf) some years back.

for me, this concept means to put pre-conditions, invariants and post-conditions of a class/method/function into a test suite and this test suite describes and tests what a user of this entity can expect of this function.

For example, say you write a function to act as a pipeline stage pipe_stage. You know that you get inputs, you transform them to produce outputs. So I typically write a test setup (known as test fixture) that creates synthetic data (gaussian, uniform, ...) and then add tests on this setup that comply and test with

• assumptions that have to hold before the pipe_stage is entered, such as the inputs need to be numpy arrays, or the inputs have to comply to a certain stage (these are pre-conditions)

• assumptions that have to hold throughout the pipe_stage executation, e.g. your input should not be deleted, some internal state of pipe_stage needs to remain fixed (like switch variables or alike) (these are invariants)

• assumptions that have to hold after the pipe_stage was executed, such as the outputs need to be numpy arrays too (these are post-conditions)

It's important to prioritize aspects to test. Otherwise, you end up testing every if clause in your implementation - which is nice for test coverage, but bad for getting on with life. ;-)

To complete the circle, a test harness is like a knight's harness (German "Harnisch"). It protects on the one hand from a foe's attack - for software this is bugs or your future self loosing track of functionality. On the other side, a harness is something rigid. It fixes things into place, so that you can better work with them and your unit tests become a kind of documentation.

[ardizzone]

I can also help with this, I think I am responsible for the graph traversal 'algorithm'.

[fdraxler]

Can we -- as an alternative to comments -- replace the internals of ReversibleGraphNet by more readable code? I had big issues debugging my own code, because stack traces were not interpretable (long recursive stack) and step-by-step-debugging was not understandable due to ids passed around instead of the underlying objects.

I don't really have an opinion on what I want to work, but I would be happy to do this and could guarantee API consistency for the Node class.

[wapu]

replace the internals of ReversibleGraphNet by more readable code

If we can pull that off, it would be amazing. But has significant potential to break things, too :sweat_smile:

[psteinb]

I think before starting such an endavour, we should write unit tests that fix the expected behavior. This at least helps locally to know if/when things break. If you provide a list, what a this class should do, then I could set this up tonight.

[wapu]

The vast majority of the code in ReversibleGraphNet is just for creation and bookkeeping of the computation graph, which is what @fdraxler is proposing to overhaul. So tests for this class should probably just create a few interesting computation graphs and check that for a given input, the output, Jacobian and inverse are computed correctly.

[fdraxler]

I think all modules that do any kind of invertible computation (i.e. all couplings, blocks, and even the graph ReversibleGraphNet) should have the same signature for forward, like the one proposed.

Then we could have one unit test that checks $f$ vs $f^{-1}$ and $df/dx = (df/dz)^{-1} = lim_{h \to 0} (f(x + h) - f(x)) / h$ exactly like @wapu said, and it works for all modules.

For backwards compatibility, one could make a legacy base module that just calls the old forward and jacobian methods depending on jac. However, most end user code should be easy to adapt, so it might not be necessary.

[fdraxler]

How about the following:

• [ ] Change all signatures, adding a side quest to checking all modules #3
• [ ] Write unit test that checks inversion and Jacobian for all modules (confirm for previous) #4

And either of:

• [ ] Minimally rewrite ReversibleGraphNet for new signature
• [ ] Rewrite ReversibleGraphNet with debug helpers and without buffers #7

[ardizzone]

I think for the average user, the worst change is the tuple output (it even thew me off this morning, as you saw). The impression I got is that not many people wrote their own modules.

So i think when you suggest

For backwards compatibility, one could make a legacy base module that just calls the old forward and jacobian methods depending on jac.

that is the wrong way of backward compatibility, and I would rather make something that only returns z, and hold the jacobian back until log_jacobian() is called. (I would also be fine with having neither, and simply putting a warning or something)

Should we go forward with this starting this evening and tomorrow morning? Building on what felix suggests, I would say

• [ ] Write an abstract base class FrEIA.modules.Operator or something, to define the new interface with the modules (@fdraxler for instance)
• [ ] Change all signatures as part of #3 (I, @ardizzone, can do this, I was planning on doing #3 tomorrow)
• [ ] Write unit test that checks inversion and Jacobian for all modules #4
• [ ] Write tests for ReversibleGraphNet (see below)
• [ ] Rework ReversibleGraphNet, including #7, returning (z,jac), more readable, etc. (@fdraxler , you sound eager to do it? :grin:)

In line with what Jakob says, some possible tests for ReversibleGraphNet:

• Splitting/merging
• Non-trivial order of operations
• Does it throw an error for cyclic graphs

(Currently, I believe it works correctly for all cases) With modules like fixed seeded permutation and fixed linear scaling, it should also be possible to work out the result on paper, and explicitly test for it.

[wapu]

Uhhh... how do you define a cyclic graph for testing? By surgically changing the inputs of earlier defined nodes to the outputs of later ones, or is there a way this can happen "naturally"?

[ardizzone]

Ah, oops! Good question! Maybe it's not that easily possible to break it in that way! :D In this case, disregard what I said!

[fdraxler]

By surgically changing, it should be possible, I guess. I thought it might be a design choice that mingling with nodes should be okay. But I can't really come up with a situation where it is useful (since we don't allow cycles anyway). Right now, I have a zero-cost check that prevents cyclic graphs.

If you say it's not, ReversibleGraphNet can be condensed to some hundred super-legible lines of code and the stack traces of wrong module instantiation and wrong inputs appear at the place of Node creation.

[wapu]

If you say it's not

What is not? :D

I think it's not particularly useful to be able to mess with the node inputs after having assembled the graph. If that's what you mean. Maybe there are use cases for it, but without knowing a good one I'm not sure it's worth any complication of the code.

Cycles in a computation graph (especially an invertible one) simply don't make sense, allowing them is not an option. But since I don't see how they could be created by accident anyway, we probably don't have to think about it too much.

[ardizzone]

Could somebody also test ist multi-GPU works? (torch DataParallel). That was the reason why we used _buffers in the first place with the old RGN, because pytorch knows how to properly split them across multiple GPUs.

[fdraxler]

The code is mainly done, but untested. I am waiting for #3 to be able to thoroughly test it.

Could somebody also test ist multi-GPU works? (torch DataParallel).

I can test with the rest.