Skip to content
This repository was archived by the owner on Nov 16, 2023. It is now read-only.

Conversation

@mstfbl
Copy link
Contributor

@mstfbl mstfbl commented Nov 13, 2019

Additional fix to Issue #360
Fixes #366

Handles the case where, with Python 2.7, string s is valid with utf_8 encoding, but is the empty string. I discovered the need for this fix as 2 test cases that were skipped before PR #359 were failing even with PR #359, due to this case with the empty string on Python 2.7.

These tests were:

  • TestNGramFeaturizer.test_ngramfeaturizer()
  • TestLightGbmClassifier.test_lightgbmclassifier()

EDIT: My observation on the error provided in TestLightGbmClassifier.test_lightgbmclassifier() was incorrect. The error is only visible in TestNGramFeaturizer.test_ngramfeaturizer(). The exact failure is here and here, shown on the Mac 2.7 build.

@mstfbl mstfbl requested a review from ganik November 13, 2019 23:45
@ganik
Copy link
Member

ganik commented Nov 13, 2019

    if (bp::extract<const char*>(str(s).encode("utf_8")).check())

What does this check() method do?


Refers to: src/NativeBridge/DataViewInterop.h:241 in 1f92b8a. [](commit_id = 1f92b8a, deletion_comment = False)

@mstfbl
Copy link
Contributor Author

mstfbl commented Nov 14, 2019

The check() method in this context is:

  template <class Ptr>
  inline bool extract_pointer<Ptr>::check() const
  {
      return m_source == Py_None || m_result != 0;
  }

According to the Boost docs here, I believe this functions checks whether the given object can be converted to a given C# type, which in this case would be <const char*>. This check is necessary to properly handle a given text from Python to C++, and later to C#.

@mstfbl
Copy link
Contributor Author

mstfbl commented Dec 16, 2019

As of commit d9b9544, the Mac Py2.7 build passes.

@mstfbl
Copy link
Contributor Author

mstfbl commented Dec 20, 2019

Closing this PR for now as we are temporarily disabling the corresponding tests for the Mac Python 2.7 builds with PR #391 .

@mstfbl mstfbl closed this Dec 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ExtendedTests failing on Mac Python 2.7 build due to NGramFeaturizer.py

2 participants