Initialization of sanction function

[angrymeir]

Hey @sergioburdisso,

as far as I understand the SS3 framework, there is an inconsistency between the initialization of SS3 and its documentation.
The initialization describes the parameter sn_m as method used to compute the sanction (sn) function [...]

However in the actual initialization the only function that changes based on the sn_m parameter is the significance function (see here).

I would be great if you could have a look at it and tell me whether I'm wrong? 😄

Best,
Florian

[sergioburdisso]

This gif perfectly describes how I feel right now:

I don't know what I was thinking when I wrote that part of the code, maybe I was hungry and was thinking about eating some pizza (?) 🤦 lol.

Anyway, thanks for reporting this, I should update the documentation and replace "sanction" with "significance", and "sn_m" with "sg_m" (Note: if you have already done it yourself, do not hesitate to create a new Pull Request, it will be appreciated and received with love ❤️ ).

[angrymeir]

Ah nice 😄
I implemented the changes, however the tests for the latest commit fail for me at Job 1.2 and Job 1.4.

Are you sure this test is implemented correctly? To me it seems odd, that there is no path given...
Could you please help me to understand the problem here?

Btw: Is there a reason that only Job 1.2 and Job 1.4 perform server testing?

[sergioburdisso]

That is really really super duper weird 😮, why failing with Python 3.5 and 3.7, and not 3.6? that is super weird 🤔.

Are you sure this test is implemented correctly? To me it seems odd, that there is no path given...

The path should be given by the mockers argument:

pyss3/tests/test_server.py

Line 215 in bc8ec0a

def test_main(mockers, mocker):

Which is defined here:

pyss3/tests/test_server.py

Lines 57 to 65 in bc8ec0a

	@pytest.fixture()
	def mockers(mocker):
	"""Set mockers up."""
	mocker.patch("webbrowser.open")
	mocker.patch.object(LT, "serve")
	mocker.patch.object(SS3, "load_model")
	mocker.patch.object(argparse.ArgumentParser, "add_argument")
	mocker.patch.object(argparse.ArgumentParser,
	"parse_args").return_value = MockCmdLineArgs

The last two lines above (line 64 and 65) return a mocked version of the "parse_args" which is defined here:

pyss3/tests/test_server.py

Lines 46 to 54 in bc8ec0a

	class MockCmdLineArgs:
	"""Mocked command-line arguments."""
	quiet = True
	MODEL = "name"
	path = dataset_path
	path_labels = None
	label = 'folder'
	port = 0

There, in line 51, the path is being set (path = dataset_path). That's the way the path should be given, which explains why it worked for all the other jobs... but why not for those Job 1.2 and Job 1.4? mmm... what I can tell by the error log, somehow the path is (or has become) None there, would it be caused by the previous "ValueError: the default category must be 'most-probable', 'unknown', or a category name (current value is 'xxxxx')" error? I don't know, in which case I would try removing or commenting out this line:

pyss3/tests/test_server.py

Line 155 in bc8ec0a

serve_args["def_cat"] = 'xxxxx' # raise ValueError

But, as I said, it is weird, the server test is not "straightforward" because it has to use threads to run the server multiple times with different arguments sending (handcrafted) HTTP requests from the main test thread to those "server threads", could it be something related to the overlapping of those asynchronous tests, somehow? Anyhow, feel free to perform the Pull Request, we will merge it and then try to locally replicate the error to trace back where this error is originating. Don't worry, before releasing a new version with these patches, we will fix these test errors, what do you think?

[angrymeir]

Ah okay now I understand where it gets its arguments from.

I just ran the tests on a plain 18.04 Ubuntu VM and all of them passed...
However ValueError: the default category must be 'most-probable', 'unknown', or a category name (current value is 'xxxxx'). still occurs - guess this doesn't have an impact on our failing tests.
Maybe it's due to the fact, that the Buildagents still use Ubuntu 16.04?

Then lets do the merge and figure out the problem after that :)

[sergioburdisso]

Perfect! Thanks for your contribution!!!

I've merged the changes and updated the Contributions section to add "code" to your contribution types 👍.
Now I'll try to locally replicate the error before releasing the new version containing your patch. 🤞 🤞 🤞

(I've just sent you an invitation to be added as a contributor, so you can get direct access to this repository from now on 😎 )

[angrymeir]

Nice! \o/
Let me know if you find anything - I'll also have a look into it again later 🔍