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

Create the header to determine the layer type #1987

Merged
merged 18 commits into from Oct 11, 2019

Conversation

@sreenikSS
Copy link
Contributor

sreenikSS commented Aug 26, 2019

  • Doesn't support all the layers yet
  • SELU hasn't been added as it identifies as ELU
@sreenikSS sreenikSS changed the title Create file Create the header to determine the layer type Aug 26, 2019
!std::is_same<T, SigmoidLayer<> >::value &&
!std::is_same<T, LogSoftMax<> >::value,
std::string>::type
LayerString(T* layer) const { return "linear"; }

This comment has been minimized.

Copy link
@zoq

zoq Aug 26, 2019

Member

On a second look we should be able to simplify this expression, by using:

LayerString(Linear<> layer) const { return "linear"; }
LayerString(Convolution <> layer) const { return "convolution"; }

This comment has been minimized.

Copy link
@sreenikSS

sreenikSS Aug 27, 2019

Author Contributor

I don't think that's possible because not all layers are implemented yet so wouldn't the compiler be unable to determine if the LayerTypes<> instances obtained from the FFN model actually have functions with the respective types overloaded?

I am also unsure about error-free passing of LayerTypes<> objects to these methods directly. Let me know what you think.

This comment has been minimized.

Copy link
@zoq

zoq Aug 27, 2019

Member

I was thinking about:

class LayerNameVisitor : public boost::static_visitor<std::string>
{
 public:
  LayerNameVisitor()
  {
  }

  std::string LayerString(LinearNoBias<>* /*layer*/) const
  {
    return "linearnobias";
  }

  template<typename T>
  std::string LayerString(T* /*layer*/) const
  {
    return "unknown";
  }

  template<typename LayerType>
  std::string operator()(LayerType* layer) const
  {
    return LayerString(layer);
  }
};

Every layer that isn't implemented is catched by:

template<typename T>
std::string LayerString(T* /*layer*/) const
{
  return "unknown";
}

This comment has been minimized.

Copy link
@sreenikSS

sreenikSS Aug 27, 2019

Author Contributor

Oh yes got it, this will work. I'll make the changes and push it. Thanks.

sreenikSS added 4 commits Aug 26, 2019
src/mlpack/methods/ann/layer_names.hpp Show resolved Hide resolved
src/mlpack/methods/ann/layer_names.hpp Show resolved Hide resolved
@@ -7,6 +7,7 @@ set(SOURCES
rnn_impl.hpp
brnn.hpp
brnn_impl.hpp
layer_names.hpp

This comment has been minimized.

Copy link
@zoq

This comment has been minimized.

Copy link
@sreenikSS

sreenikSS Aug 30, 2019

Author Contributor

Sure, I'll get it done tomorrow, pushing the changes here for now

@rcurtin rcurtin added this to the mlpack 3.2.0 milestone Aug 31, 2019
}

/*
* Return the name of the given layer of type AtrousConvolution as a string

This comment has been minimized.

Copy link
@zoq

zoq Sep 1, 2019

Member

Missing stop at the end of the function and parameter descriptions.

This comment has been minimized.

Copy link
@sreenikSS

sreenikSS Sep 4, 2019

Author Contributor

@zoq Could you take a look once? I looked into the test, it seems like it wasn't compiling because of some other issue (though still related to boost). I have pushed the test along with this but can you tell me how to run tests in general (like, what compilation statement do you use)?

This comment has been minimized.

Copy link
@zoq

zoq Sep 4, 2019

Member

I'll add the new cpp file to the CMakeList.txt : https://github.com/mlpack/mlpack/blob/master/src/mlpack/tests/CMakeLists.txt right after lars_test.cpp should work just fine. Afterwards make should pick up the the file and include it into the build process.

If you make a test suite called "TestSuite" (BOOST_AUTO_TEST_SUITE(TestSuite)), and
then build mlpack_test (make mlpack_test) or just make, you can run only the
tests in that test suite with bin/mlpack_test -t TestSuite. A
specific test case called TestCase (BOOST_AUTO_TEST_CASE(TestCase))
could be run with bin/mlpack_test -t TestSuite/TestCase.

LayerTypes<> unsupportedLayer = new BilinearInterpolation<>(); /* Bilinear
interpolation is not yet supported by the string converter */

BOOST_REQUIRE_EQUAL(boost::apply_visitor(LayerNameVisitor(),

This comment has been minimized.

Copy link
@zoq

zoq Sep 4, 2019

Member

Haven't tested it but perhaps:
BOOST_REQUIRE(boost::apply_visitor(LayerNameVisitor(), atrousConvolution) == "atrousconvolution");

will solve the build issue.

@zoq

This comment has been minimized.

Copy link
Member

zoq commented Sep 9, 2019

Looks like this didn't work, I will have to take a closer look into the issue, I guess what we could test is to do the comparison outside the BOOST check, maybe that solves the problem.

Copy link
Member

rcurtin left a comment

I didn't try to reproduce the build error, but maybe binding the result of boost::apply_visitor() to a std::string could work here, then you could use BOOST_REQUIRE_EQUAL()? i.e.

std::string name = boost::apply_visitor(LayerNameVisitor(), ...);
BOOST_REQUIRE_EQUAL(name, "...");

At the very least, either that or Marcus's suggestion will make it easier to know whether or not the build error is something strange caused by the Boost Unit Test library's macros (which does happen sometimes...).

LayerTypes<> sigmoidLayer = new SigmoidLayer<>();
LayerTypes<> logSoftMax = new LogSoftMax<>();
LayerTypes<> unsupportedLayer = new BilinearInterpolation<>(); /* Bilinear
interpolation is not yet supported by the string converter */

This comment has been minimized.

Copy link
@rcurtin

rcurtin Sep 12, 2019

Member

Probably to fix the style error you could just have this comment as a single-line comment above the definition of unsupportedLayer. 👍

src/mlpack/methods/ann/layer_names.hpp Show resolved Hide resolved
@@ -0,0 +1,316 @@
/**
* @file layer_names.hpp

This comment has been minimized.

Copy link
@rcurtin

rcurtin Sep 12, 2019

Member

It might make sense to split this into an _impl.hpp file also; up to you.

src/mlpack/methods/ann/layer_names.hpp Show resolved Hide resolved
src/mlpack/tests/layer_names_test.cpp Show resolved Hide resolved
@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Sep 21, 2019

@sreenikSS have you had a chance to look into this? Or should I remove it from the 3.2.0 release? It's one of two PRs remaining. It seems to me like it wouldn't take too long to fix. 👍

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Sep 23, 2019

The build failure is actually because the visitor doesn't handle the MoreTypes layer (see #1978). All that needs to be done to fix it is to add the following method to the LayerNameVisitor:

  std::string operator()(MoreTypes layer) const
  {
    return layer.apply_visitor(*this);
  }

Looks like a test will need to be fixed too:

{ ryan@masterblaster }{ ~/src/mlpack/build } $ bin/mlpack_test -t LayerNamesTest
Running 1 test case...
/home/ryan/src/mlpack/src/mlpack/tests/layer_names_test.cpp(72): fatal error: in "LayerNamesTest/LayerNameVisitorTest": critical check boost::apply_visitor(LayerNameVisitor(), flexibleReLU) == "flexiblerelu" has failed

*** 1 failure is detected in the test module "mlpackTest"

(the current implementation prints frelu not flexiblerelu)

BOOST_REQUIRE(boost::apply_visitor(LayerNameVisitor(),
unsupportedLayer) == "unsupported");
// Delete all instances.
delete &atrousConvolution;

This comment has been minimized.

Copy link
@rcurtin

rcurtin Sep 23, 2019

Member

Actually I think that we have to be a little special in how we handle this. Since atrousConvolution and the others are of type LayerTypes<> (which is itself a boost::variant<> that holds a pointer to layer types), we have to use a visitor to delete them too. I think this will work:

boost::apply_visitor(DeleteVisitor(), atrousConvolution);

This comment has been minimized.

Copy link
@zoq

zoq Sep 23, 2019

Member

Not sure, but maybe it would be simpler if we go with the other idea? Adding static std::string layerName to each layer?

This comment has been minimized.

Copy link
@walragatver

walragatver Sep 24, 2019

Contributor

Not sure. But I think you are correct. I was required to call visitor for boost::<variant> while writing tests for bias visitor.

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Sep 26, 2019

@walragatver pointed out in IRC today that we shouldn't wait on the release any longer, as mlpack 3.1.1 downloads ensmallen-latest.tar.gz---which as of ensmallen 2.10.0 no longer works! I agreed, so I've gone ahead and released 3.2.0 tonight (which doesn't have the same issue as 3.1.1 did with respect to the ensmallen version), and I've moved this to the 3.2.1 milestone. Once this is finished and merged we can easily issue a patch release. 👍

@rcurtin rcurtin modified the milestones: mlpack 3.2.1, mlpack 3.2.2 Sep 27, 2019
@sreenikSS

This comment has been minimized.

Copy link
Contributor Author

sreenikSS commented Oct 1, 2019

Sounds good. I haven't had time to get back to this after reading your comment a few days back (organised a couple of events at college). Anyway, I hope this would be fixed before the next release :)

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Oct 9, 2019

@sreenikSS if you're okay with me pushing this to your branch, I'm happy to finish it up so we can get it merged. Just let me know what you're thinking. 👍

@sreenikSS

This comment has been minimized.

Copy link
Contributor Author

sreenikSS commented Oct 9, 2019

Let's see if this fix works. Extremely sorry for the delay for such a simple fix.

LayerTypes<> pReLU = new PReLU<>();
LayerTypes<> sigmoidLayer = new SigmoidLayer<>();
LayerTypes<> logSoftMax = new LogSoftMax<>();
// Bilinear interpolation is not yet supported by the string converter

This comment has been minimized.

Copy link
@zoq

zoq Oct 10, 2019

Member

Pedantic style issue, missing stop at the end.

@@ -0,0 +1,135 @@
/**
* @file layer_names_test.cpp
*

This comment has been minimized.

Copy link
@zoq

zoq Oct 10, 2019

Member

Feel free to add your name here (@author).

Copy link
Member

rcurtin left a comment

From my end this looks good, thanks for the contribution. 👍 If you don't get a chance we can handle the two comments @zoq pointed out during merge.

@zoq
zoq approved these changes Oct 11, 2019
Copy link
Member

zoq left a comment

Looks good to me, no more comments from my side.

@zoq zoq merged commit 9bde8fe into mlpack:master Oct 11, 2019
5 of 6 checks passed
5 of 6 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
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
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.