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

[SYSTEMDS-3545] img_cutout_linearized #1845

Closed
wants to merge 8 commits into from

Conversation

slnkahveci
Copy link
Contributor

Cutout method for row linearized images. This functionality is being developed as part of a student project.

@Baunsgaard
Copy link
Contributor

Hi @kahvecioglu

Welcome to open source, I can suggest looking at the PR #1838
Here the initial commits adds the new builtin function, and have some of the same steps that you need to go through adding tests for your builtin.

@Baunsgaard
Copy link
Contributor

Hi @kahvecioglu,

How is this PR going, is there some uncommitted progress?

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.

Good progress,
some minor comments.

bin/systemds Outdated
@@ -396,7 +396,7 @@ fi
JARNAME=$(basename "$SYSTEMDS_JAR_FILE")

# relative path to jar file
SYSTEMDS_JAR_FILE=$(realpath --relative-to=. "$(dirname "$SYSTEMDS_JAR_FILE")")${DIR_SEP}${JARNAME}
SYSTEMDS_JAR_FILE=$(grealpath --relative-to=. "$(dirname "$SYSTEMDS_JAR_FILE")")${DIR_SEP}${JARNAME}
Copy link
Contributor

Choose a reason for hiding this comment

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

does this fix it for mac ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does. Should I change it back in my next commit?

scripts/builtin/img_cutout_linearized.dml Show resolved Hide resolved
# ------------------------------------------------------------------------------------------


m_img_cutout_linearized = function(Matrix[Double] img_in, Integer x, Integer y, Integer width, Integer height, Double fill_value, Integer s_cols, Integer s_rows) return (Matrix[Double] img_out) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i would like some newlines to break this line.

# height Height of the rectangle (must be positive)
# fill_value The value to set for the rectangle
# s_cols Width of a single image
# s_rows Height of a single image
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

scripts/builtin/img_cutout_linearized.dml Show resolved Hide resolved
#
# OUTPUT:
# ------------------------------------------------------------------------------------------
# img_out Output images as linearized 2D matrix with top left corner at [1, 1]
Copy link
Contributor

Choose a reason for hiding this comment

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

for not obvious reasons we need to spaces between the img_out and the beginning of the documentation

import java.util.Random;
// if A should be generated one row at a time
//import java.util.stream.Stream;
//import java.util.stream.DoubleStream;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented lines like this.

int width = random.nextInt(s_cols - x) + 1;
int height = random.nextInt(s_rows - y) + 1;
int fill_color = random.nextInt(256);
int n_imgs = random.nextInt(100) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

this random behavior is not good, since each test then behaves randomly and we cannot reproduce.

Use a parameterized class instead like in PR #1903

* double[] row = Stream.of(matrix).flatMapToDouble(DoubleStream::of).toArray();
* A[i] = row;
* }
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While generating the input, does 'sparsity' refer to the sparsity of a single row image or the entire matrix? I left that block in there because I wasn't sure about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

sparsity refer to the entire matrix generated, The generated matrix is trying to get the sparsity you request, but will most likely never hit it exactly.

@slnkahveci
Copy link
Contributor Author

@Baunsgaard The script and the test are working but the checks keep failing. (which is also the case for #1916 ) What should I do to resolve this issue?

@Baunsgaard
Copy link
Contributor

@Baunsgaard The script and the test are working but the checks keep failing. (which is also the case for #1916 ) What should I do to resolve this issue?

Sometimes some of the tests fail online, we rerun in such cases. You can always look into the ones that fail and try to decipher if it is related to your code.

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

Successfully merging this pull request may close these issues.

2 participants