shogun-toolbox / shogun

Shōgun
http://shogun-toolbox.org
BSD 3-Clause "New" or "Revised" License
3.03k stars 1.04k forks source link

class members with the parameter names (#5042) #5094

Closed JasonSurendran closed 4 years ago

JasonSurendran commented 4 years ago

Adding parameter name to CrossValidation.h and parameter registration to CrossValidation.cpp

gf712 commented 4 years ago

Thanks! Feel free to add more to this PR, this exactly what needs to be done :)

JasonSurendran commented 4 years ago

Added in parameters names/registrations to 10 .h/.cpp file pairs

karlnapf commented 4 years ago

All m_prefixes (only in the strings, not in the member variable definition). Also Everywhere in the repository (if referring to this class), not just in this file (the tests will tell you)

On Fri, 10 Jul 2020 at 16:58, Jason Surendran notifications@github.com wrote:

@JasonSurendran commented on this pull request.

In src/shogun/evaluation/SigmoidCalibration.h https://github.com/shogun-toolbox/shogun/pull/5094#discussion_r452931940 :

@@ -139,6 +139,15 @@ namespace shogun float64_t m_sigma; /* Stopping criteria of search / float64_t m_epsilon;

  • ifndef SWIG

  • public:
  • static constexpr std::string_view kSigmoidAs = "m_sigmoid_as";

should we only remove the m_prefix from "m_sigmoid_as" or from all of the names with the m_prefix on the file?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/shogun-toolbox/shogun/pull/5094#discussion_r452931940, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFKVP3MJT4ZFUUBXUBKQVLR243C7ANCNFSM4OVU5RUQ .

-- Sent from my phone

karlnapf commented 4 years ago

Looks good now. Let's see what the tests say.

JasonSurendran commented 4 years ago

Not sure what the error refers to: https://dev.azure.com/shogunml/shogun/_build/results?buildId=3860&view=logs&j=089c709a-44eb-5f6e-96e7-15e9ee1ff5bf&t=2da3e16b-a2b2-5f01-2cbe-a20d9528195b&l=211

karlnapf commented 4 years ago

Not sure what the error refers to: https://dev.azure.com/shogunml/shogun/_build/results?buildId=3860&view=logs&j=089c709a-44eb-5f6e-96e7-15e9ee1ff5bf&t=2da3e16b-a2b2-5f01-2cbe-a20d9528195b&l=211

notebooks can be a bit unstable in the CI. Just grep for the name changes in them to see whether you need to adjust something. Other tests look good

JasonSurendran commented 4 years ago

It passes all checks now

karlnapf commented 4 years ago

Thanks a lot for this! :)