[jira] Issue Comment Edited: (MAHOUT-184) Code tweaks for .df.* code

["Deneche A. Hakim (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/MAHOUT-184?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12761819#action_12761819 ] 

Deneche A. Hakim edited comment on MAHOUT-184 at 10/2/09 11:32 PM:
-------------------------------------------------------------------

I'm having trouble applying the patch, got a lot of: "Hunk #1 FAILED at XX."
I inspected the patch without applying it and it look fine, although I have two comments:

* comment 1:
in core/main/.../df/node/Node.java you added the following comment:
{code}
   private int node2Type() {
+    // TODO shouldn't subclasses override this method then??
{code}

the problem is that node2type() is used when writing a Node to actually store its class in the form of an Integer. The static method Node.read(DataInput in) is used to read the node delegating the reading to the correct implementation. Overriding node2Type() without modifying Node.read(DataInput in) is a certain way to have headaches !!!
A better solution is to store the class name and use class loaders, but for now I don't really need it.

* comment 2:
in core/src/test/java/org/apache/mahout/df/data/DataLoaderTest.java you made the following change:

{code}
+  protected static void checkCategorical(double[][] source, List<Integer> missings,
       Data loaded, int attr, int aId, double oValue, double nValue) {
     int lind = 0;
 
@@ -286,7 +277,7 @@
         continue;
 
       if (source[index][attr] == oValue) {
-        assertTrue(nValue == loaded.get(lind).get(aId));
+        assertEquals(nValue, loaded.get(lind).get(aId), 0.0);
{code}

I think you meant :

{code}
+        assertEquals(nValue, loaded.get(lind).get(aId)); 
{code}


      was (Author: adeneche):
    I'm having trouble applying the patch, got a lot of: "Hunk #1 FAILED at XX."
I inspected the patch without applying it and it look fine, although I have two comments:

* comment 1:
in core/main/.../df/node/Node.java you added the following comment:
{code}
   private int node2Type() {
+    // TODO shouldn't subclasses override this method then??
{/code}

the problem is that node2type() is used when writing a Node to actually store its class in the form of an Integer. The static method Node.read(DataInput in) is used to read the node delegating the reading to the correct implementation. Overriding node2Type() without modifying Node.read(DataInput in) is a certain way to have headaches !!!
A better solution is to store the class name and use class loaders, but for now I don't really need it.

* comment 2:
in core/src/test/java/org/apache/mahout/df/data/DataLoaderTest.java you made the following change:

{code}
+  protected static void checkCategorical(double[][] source, List<Integer> missings,
       Data loaded, int attr, int aId, double oValue, double nValue) {
     int lind = 0;
 
@@ -286,7 +277,7 @@
         continue;
 
       if (source[index][attr] == oValue) {
-        assertTrue(nValue == loaded.get(lind).get(aId));
+        assertEquals(nValue, loaded.get(lind).get(aId), 0.0);
{/code}

I think you meant :

{code}
+        assertEquals(nValue, loaded.get(lind).get(aId)); 
{/code}

  
> Code tweaks for .df.* code
> --------------------------
>
>                 Key: MAHOUT-184
>                 URL: https://issues.apache.org/jira/browse/MAHOUT-184
>             Project: Mahout
>          Issue Type: Improvement
>            Reporter: Sean Owen
>            Assignee: Sean Owen
>            Priority: Minor
>             Fix For: 0.2
>
>         Attachments: Tweaks_to__df__.patch
>
>
> This follows on my last email to the mailing list, and code inspection. It's big enough I made a patch. No surprises I hope given the consensus on code style and practice. Might be some good takeaways in here, or points for further discussion.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.