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

Added support for Loading image #1903

Merged
merged 42 commits into from Aug 10, 2019

Conversation

@MuLx10
Copy link
Member

commented May 28, 2019

Added functionality for loading images with the help of stb_image.h.
Currently supports jpg,png,tga, bmp, psd, gif, hdr, pic, pnm.

Reamining Task

  • Find stb_image in CMakeList.
  • Change the download url for stb_image.h
  • Save image.
  • Add documentation.
@zoq

This comment has been minimized.

Copy link
Member

commented May 28, 2019

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/tests/image_load_test.cpp Outdated Show resolved Hide resolved
MuLx10 added 2 commits May 28, 2019
Changed to std:experimental::filesystem.
Incorporated style checks.
@MuLx10 MuLx10 force-pushed the MuLx10:LoadImage branch from 8e461f0 to b95450d May 30, 2019
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image_impl.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image_impl.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image_impl.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image_impl.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image_impl.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image_impl.hpp Outdated Show resolved Hide resolved
@zoq zoq removed the s: unanswered label May 31, 2019
@jeffin143

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

Added functionality for loading images with the help of stb_image.h.
Currently supports jpg,png,tga, bmp, psd, gif, hdr, pic, pnm.

@MuLx10 Just curious , suppose just like loading images ,can we save too using this.? For suppose we have mat and simply we can save it as an image.?

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

left a comment

This isn't really a complete review---I just sort of wandered through the code and wrote down some notes. I'll try and do a more complete review after I get some sleep. :)

The idea is great and I'm excited to get this merged. But, how do you think we should document this support so that new users can find out about it? I wonder if one of the tutorials about data loading/saving in doc/guide/ is a place to mention this. In any case, I don't think we need to do that for this PR, but in the long run we should definitely have a plan so users can know that they can work with big sets of images easily. 👍

LICENSE.txt Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image_impl.hpp Outdated Show resolved Hide resolved
@zoq

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

I like where this Is going, excited to see this merged.

@MuLx10 MuLx10 force-pushed the MuLx10:LoadImage branch from 0840bf8 to dc2c6b8 Jun 2, 2019
src/mlpack/core/data/load_image_impl.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image_impl.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
src/mlpack/core/data/load_image.hpp Outdated Show resolved Hide resolved
@MuLx10

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

Added functionality for loading images with the help of stb_image.h.
Currently supports jpg,png,tga, bmp, psd, gif, hdr, pic, pnm.

@MuLx10 Just curious , suppose just like loading images ,can we save too using this.? For suppose we have mat and simply we can save it as an image.?

No, not currently, but I can add support for saving image as well.

@jeffin143

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Added functionality for loading images with the help of stb_image.h.
Currently supports jpg,png,tga, bmp, psd, gif, hdr, pic, pnm.

@MuLx10 Just curious , suppose just like loading images ,can we save too using this.? For suppose we have mat and simply we can save it as an image.?

No, not currently, but I can add support for saving image as well.

@zoq , will it be good , If we add that support as well, we can visualize what image may be a gan model outputs or may be any other use case

@zoq

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Would be nice to save images as well, not sure it should be part of the PR. @MuLx10 do you think this is a simple addition?

@@ -78,6 +78,10 @@ if(CMAKE_COMPILER_IS_GNUCC OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Wextra")
endif()

# To remove unused functions warnings.
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused-function")

This comment has been minimized.

Copy link
@zoq

zoq Jul 26, 2019

Member

That works for me, @rcurtin does this work for you as well?

@MuLx10 MuLx10 force-pushed the MuLx10:LoadImage branch from 5e7f328 to 58d6a8e Jul 26, 2019
@MuLx10

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2019

@zoq please restart the build, thank you.

@zoq

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

Restarted.

@zoq

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@mlpack-jenkins test this please

1 similar comment
@zoq

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@mlpack-jenkins test this please

@zoq

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@MuLx10 Sorry for the Jenkins related comments, I used the PR to figure out an issue with the Static Analysis job.

@zoq

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Looks good the, travis test issue isn't caused by this PR.

@zoq
zoq approved these changes Jul 30, 2019
Copy link
Member

left a comment

This looks really good to me, thanks for all the work you put into this, no more comments from my side.

@mlpack-bot
mlpack-bot bot approved these changes Jul 31, 2019
Copy link

left a comment

Second approval provided automatically after 24 hours. 👍

@rcurtin
rcurtin approved these changes Aug 1, 2019
Copy link
Member

left a comment

Looks great to me. Thanks for the hard work @MuLx10! I have a few comments but I don't want to hold up the merge process. If you can handle them before merge it would be great, but if not I can provide a patch, just let me know.

doc/tutorials/image/image.txt Outdated Show resolved Hide resolved
ImageInfo& info,
const bool fatal = false,
const bool transpose = true);
#endif // HAS_STB.

This comment has been minimized.

Copy link
@rcurtin

rcurtin Aug 1, 2019

Member

It could be really useful to also have something like

#else 
struct ImageInfo { };

template<typename eT>
bool Load(const std::string& filename,
          arma::Mat<eT>& matrix,
          ImageInfo& info,
          const bool fatal = false,
          const bool transpose = true)
{
  throw std::runtime_error("Load(): HAS_STB is not defined, so STB is not available and images cannot be loaded!");
}

template<typename eT>
bool Load(const std::vector<std::string>& files,
          arma::Mat<eT>& matrix,
          ImageInfo& info,
          const bool fatal = false,
          const bool transpose = true)
{
  throw std::runtime_error("Load(): HAS_STB is not defined, so STB is not available and images cannot be loaded!");
}

Also it would be useful to add documentation to the top of the definitions here, so that Doxygen will pick it up.

ImageInfo& info,
const bool fatal = false,
const bool transpose = true);
#endif // HAS_STB.

This comment has been minimized.

Copy link
@rcurtin

rcurtin Aug 1, 2019

Member

Same comment here as in Load() when HAS_STB is not defined.

This comment has been minimized.

Copy link
@MuLx10

MuLx10 Aug 7, 2019

Author Member

Okay.

This comment has been minimized.

Copy link
@rcurtin

rcurtin Aug 7, 2019

Member

@MuLx10 Thanks---let us know when you're done with further changes for this PR. I think then we can merge it. 👍

Timer::Stop("saving_image");
if (fatal)
Log::Fatal << e.what() << std::endl;
else

This comment has been minimized.

Copy link
@rcurtin

rcurtin Aug 1, 2019

Member

Agreed, I think I wrote a lot of the offending code with the if and else, but the else isn't needed. If I see it in the future I'll change it and open a PR. :)

@rcurtin

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Ooh, also, we should update HISTORY.md. :)

@MuLx10 MuLx10 force-pushed the MuLx10:LoadImage branch from d2c0f0f to 25a0a3c Aug 1, 2019
@zoq

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Thanks for looking into the comments; the last commit introduced some style issues, I guess if you can fix those issues as well, we are about ready to merge this in.

@zoq

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

@MuLx10 let me know if you need any help with the style issues.

@zoq

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Nice, I just restarted the travis build since there was an issue with the macOS build, not sure, it is related with the changes from the code. But easy to rerun the test.

@MuLx10

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

@rcurtin @zoq I have to make a few changes, please don't merge the PR now. Thanks.

@MuLx10

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

@rcurtin @zoq I have added the changes. Once the build is complete, we are good to go.

@rcurtin rcurtin merged commit 278a7da into mlpack:master Aug 10, 2019
6 checks passed
6 checks passed
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

@MuLx10 awesome contribution, thanks so much for the hard work on this. 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.