Add Image support #6
Conversation
Co-authored-by: Toshal Agrawal <37237262+walragatver@users.noreply.github.com>
Co-authored-by: Ryan Birmingham <birm@rbirm.us>
examples/image.md
Outdated
| start = std::chrono::system_clock::now(); | ||
| FileWriter f1("temp"); | ||
|
|
||
| ifstream fin("test_image.png", ios::binary); |
There was a problem hiding this comment.
I think it's better to add this image in the examples/assets folder. Let me know what you think.
There was a problem hiding this comment.
Hi @jeffin143,
The PR looks good now except the slowness of logging arma::mat. I think let's handle its optimization in different PR. I will open an issue for it after merging this PR.
I have left some comments. Let me know what you think.
| } | ||
| while ((entry = readdir(dir)) != NULL) | ||
| { | ||
| if (entry->d_name != "." && entry->d_name != ".." |
There was a problem hiding this comment.
Hey I think it's better we remove _preprocess directory after every image logging is done. It would help in less diskusage of user. Let me know what you think.
There was a problem hiding this comment.
Umm, the directory would be empty, so i think it is ok, I think so if it is outside the we can use it for other purpose too , and be sure that all the files are deleted and won't face the issue which we were facing when you ported this function outside the test to main file
There was a problem hiding this comment.
This doesn't remove the directory in my os. But let's keep it for now.
|
Hi @jeffin143, I am not sure, but should we also take care of |
There was a problem hiding this comment.
Hi @jeffin143, I would just you to handle the OpenMP warning in a different PR. As it's a warning we can tackle it afterward. Let's not hold this PR from merging because of the error generated while handling a warning. Also, This probably would be the last review.
Co-authored-by: Toshal Agrawal <37237262+walragatver@users.noreply.github.com>
Done, That was a typo mistake |
|
I think you missed moving "examples/scaler.md" to "examples/scalar.md" |
There was a problem hiding this comment.
Hey @jeffin143, I have left some comments. I am having some doubts about the image support. Please let me know your thoughts about it.
| ss.str(""); | ||
| fin.close(); | ||
| mlboard::SummaryWriter<mlboard::FileWriter>::Image( | ||
| "Test Image", 1, encodedImages, 512, 512, f1, "Sample Image", |
There was a problem hiding this comment.
Hey, 512 and 512 is the size of the input image? Or it is of the image which will get displayed in the dashboard? I saw the assets folder the image dimensions are quite different.
There was a problem hiding this comment.
it is of the image which will get displayed in the dashboard, Currently you can give 0,0 also it doesn't make a difference, It will only make a difference I think so from next release because currently tensor board ignores those values, I have no clue why
There was a problem hiding this comment.
Okay, If I am not wrong. You are only retrieving the height and width of the image from the imageInfoobject. I think it's better we remove the use of ImageInfo from the API because the user can also enter the height and width manually. Let me know what you think.
There was a problem hiding this comment.
mlpack::data::Save(fileNames, matrix, info, false);
This line uses it, we need the channels to save it and then while reading it back we need not need it
There was a problem hiding this comment.
Oh yeah, Thanks for the info. That line got overlooked.
Co-authored-by: Toshal Agrawal <37237262+walragatver@users.noreply.github.com>
No description provided.