eMoflon / benchmarx

Infrastructure for implementing benchmarx: benchmarks for bidirectional transformation (bx) tools. Also contains a collection of example benchmarx and test runners for various and diverse bx tools.
GNU General Public License v3.0
9 stars 12 forks source link

Review: NMF #62

Closed tsdh closed 6 years ago

tsdh commented 7 years ago

Here is my review before I'll submit the evaluation sheet this evening.

Sadly, I haven't been able to run the solution because I don't have access to a Windows machine, so this review is only based on the paper and transformation source code.

And, the paper doesn't really help in understanding the solution. There's no "big picture" discussing the overall transformation strategy (like, we're essentially considering families as collections and have an add operation which adds to the right role of a family based on temporary stereotypes we gather beforehand).

Section 2 is much too brief. If you already are an category theory expert, you might nod your head and go on but if you are not, this section doesn't help at all. A less formal and more practical explanation of lenses would be more appropriate.

Then section 3 presents some parts of the actual solution. I've had a hard time mapping the NMF source code to lenses/synchronization blocks from section

  1. So when a synchronization block is an 8-tuple, what are the corresponding components in the source code of some SynchronizationRule?

And no single rule is explained completely. Just by looking at it, I'd guess that MemberToMember (why not MemberToPerson?) synchronizes a family member with a person and enforces that the result of member.GetFullName() must equal person.Name. (How would you express that multiple values must match?) Well, and since the type argument IPerson is abstract, I guess this rule is somehow not instantiable itself.

I somehow grasp how the MemberToMale and MemberToFemale rules work. This MarkInstantiatingFor() seems to be some kind of rule extension. And in the method CreateLeftOutput() some extra information is added to the member which can then be used later on to assign it to the right role of a family.

The paper doesn't list which tests pass (expectedly or unexpectedly) and which ones fail, just that some incremental tests fail because some changes like moving a family member are not transcribed as a move operation but as deletion and insertion.

So all in all, I think the solution is ok although the description could be improved.

georghinkel commented 7 years ago

Let me try to clear some of the confusions. I did not explain too much in detail as I wanted to keep the open-peer-reviewed version close to the post-proceedings version.

The proposed overall picture "we're essentially considering families as collections and have an add operation which adds to the right role of a family based on temporary stereotypes we gather beforehand" is mostly correct, but I think it only describes a part of the solution.

Although a synchronization block is an octuple, actually only 2 or 3 items are specified as the remaining can be inferred from the context. For example in the code

SynchronizeMany(SyncRule <MemberToMember >(),
                            fam => new  FamilyMemberCollection(fam),
                            persons => persons.Persons); 

The types A=FamilyRegister, B=FamilyMember, C=PersonRegister, D=Person are completely inferred by \Phi{A-C}=FamilyRegisterToPersonRegister (in the scope of which the synchronization block is defined) and \Phi{B-D}=MemberToMember. Then only the two lenses must be specified.

A synchronization block is not the same as a synchronization rule as the latter is rather an isomorphism + a set of synchronization blocks.

The fact that IPerson is abstract is not a problem for NMF, it simply instantiates the default implementation type Person, if necessary. If you wanted multiple values to match, you either specify another synchronization block or use a multi-valued synchronization block such as for the members in the familyregister.

MemberToPerson certainly is a better name for that transformation rule.

Currently, the move tests and the dynamic rename tests are the only ones that fail. All other batch and incremental tests pass.

tsdh commented 7 years ago

Ok, thanks. That clears up much of the confusion. 👍