Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Method for image transformation matrix generation (more evualtion needed) #2057

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

mark-p4
Copy link

@mark-p4 mark-p4 commented Jul 28, 2024

First implementation for a method to generate the transformation matrix is implemented and the code was already cleaned up. Multiple test cases implemented which work fine, but no "breaking cases" implemented yet. not yet part of any previous dml scrips (image_transform linearized). Within the script "image_transformation_matrix" a boolean can be changed to switch from dml script execution to internal execution.

mark-p4 added 20 commits June 8, 2024 20:36
…iltin function name, and a simple test case to trace the namespace error from a function that is not yet fully implemented, also corrected wront initial test
… have five inputs and no longer a matrix with dimensions inside.
… exceptions to trace the necessary additions
…java, accessible through MultiReturnComplexMatrixBuiltinCPInstruction and LibCommonsMath in its own class LibMatrixIMGTransform
…r on or depricate the original dml file all together
…rm_linearized.dml, img_transform_matrix.dml, img_transform_linearized.dml
Copy link

codecov bot commented Jul 28, 2024

Codecov Report

Attention: Patch coverage is 77.11864% with 27 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@5015f63). Learn more about missing BASE report.

Files Patch % Lines
...rc/main/java/org/apache/sysds/hops/FunctionOp.java 30.00% 8 Missing and 6 partials ⚠️
...sds/runtime/matrix/data/LibMatrixIMGTransform.java 87.50% 4 Missing and 5 partials ⚠️
...apache/sysds/parser/BuiltinFunctionExpression.java 73.33% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2057   +/-   ##
=======================================
  Coverage        ?   68.91%           
  Complexity      ?    40839           
=======================================
  Files           ?     1442           
  Lines           ?   161983           
  Branches        ?    31471           
=======================================
  Hits            ?   111634           
  Misses          ?    41290           
  Partials        ?     9059           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mark-p4
Copy link
Author

mark-p4 commented Jul 29, 2024

chart (1)
chart

evaluated performance based on 3 test runs each and internal vs execution via script for generating the transformation matrix, code of LibMatrixIMGTransform is cleaned up and ready to go, execution via image_matrix_transform possible and documented with particulars. Buxfixes for image_transform_linearized necessary as the base implementation is not working besides default parameters.

….dml script, where it works for certain tests but not for others
Copy link
Contributor

@Baunsgaard Baunsgaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it looks good, however we have a bunch of cleanup elements to do before this can be merged. If you look at the comments, and resolve any, then feel free to mark the element as resolved.

@@ -85,7 +85,8 @@ m_img_transform_linearized = function(Matrix[Double] img_in, Integer out_w, Inte
ind= matrix(seq(1,ncol(xs),1),1,ncol(xs))
z = table(xs, ind)
output = ys%*%z

print(nrow(img_in))
print(ncol(img_in))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove print statements in the DML scripts.

Furthermore, could we change this DML script to use your new builtin feature?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is implemented since today, had to work out a problem

z = table(xs, ind)
zMat = transMat
isFillable = as.logical(as.double(min(index_vector) == 0))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a newline in the end of files.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file deleted, as img_transform_test is being used instead


m_img_transform_matrix = function(Matrix[Double] transMat, Integer orig_w, Integer orig_h, Integer out_w, Integer out_h)
return (Matrix[Double] zMat, Matrix[Double] isFillable){
#orig_w = as.scalar(dimMat[1,2])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a convention of 2 space indentations of our builtin scripts.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

# zMat transformation matrix to be multiplied with for the linearized image
# transformation function (arbitrary naming)
# isFillable returns a boolean which indicates if cells need to be filled (with fillvalue),
in this case the image is extended, otherwise the original image is used
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is missing the hashtag

@@ -0,0 +1,52 @@
#-------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test scripts is not allowed in the builtin folder, please move this to the testing folder in /src/test/scripts/functions/builtin.
You can then source it and use the functionality without it having to be a dedicated builtin function.

programArgs = new String[]{"-nvargs", "transMat=" + input("transMat"), "dimMat=" + input("dimMat"), "out_file=" + output("B_x"), "--debug"};


runTest(true, fails, null, -1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify the results.

}


}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newlne in the end.


transformed = img_transform_linearized(input, out_w, out_h, a, b, c, d, e, f, fill_value, s_cols, s_rows)
#affineMat = matrix(0,rows=3, cols=3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to clean these tests up, such that we do not have this commented code.



#print(toString(affineMat))
#print(toString(dimMat))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and remove print statements not used.


#print(toString(zMat))
print(nrow(zMat))
print(ncol(zMat))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove print statements not used.

@Baunsgaard
Copy link
Contributor

if you address the comments, simply mark them as resolved.

@mark-p4
Copy link
Author

mark-p4 commented Jul 30, 2024

ok thanks, ill be back tonight do continue to implement improvements based on your feedback, thanks

@mark-p4
Copy link
Author

mark-p4 commented Jul 30, 2024

only intermedia push, some feedback is still not implemented, but the image_transform_linearized.dml script now uses internally implemented transformation matrix generation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants