-
Notifications
You must be signed in to change notification settings - Fork 44
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
Initialization of sanction function #11
Comments
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 ❤️ ). |
Ah nice 😄 Are you sure this test is implemented correctly? To me it seems odd, that there is no path given... Btw: Is there a reason that only Job 1.2 and Job 1.4 perform server testing? |
That is really really super duper weird 😮, why failing with Python 3.5 and 3.7, and not 3.6? that is super weird 🤔.
The path should be given by the Line 215 in bc8ec0a
Which is defined here: Lines 57 to 65 in bc8ec0a
The last two lines above (line 64 and 65) return a mocked version of the "parse_args" which is defined here: Lines 46 to 54 in bc8ec0a
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:Line 155 in bc8ec0a
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? |
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... Then lets do the merge and figure out the problem after that :) |
Perfect! Thanks for your contribution!!! I've merged the changes and updated the Contributions section to add "code" to your contribution types 👍. (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 😎 ) |
Nice! \o/ |
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
The text was updated successfully, but these errors were encountered: