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

Small changes to allow IC to use MixedChiSquare; also a quick hack to… #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stewartsiu
Copy link

… ChiSquared test to allow zero contingency entry, so that it can be used for testing.

… ChiSquared test to allow zero contingency entry, so that it can be used for testing.
@@ -81,6 +81,7 @@ def __init__(self, y, x, z, X, alpha, variable_types={}, burn=1000, thin=10, bin
self.x = x
self.y = y
self.z = z
print '\nCreating indep test for x, y, z = ',x,y,z
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @stewartsiu ! We shouldn't have print statements outside of exceptions!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, line 32 needs a variable_types={} kwarg!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akelleh, Why line 32 (it is a class definition)? If you meant line 33, then wouldn't additional code be needed to handle variable_types?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right! line 33. No additional code needs to be added to handle it. It's required since the ICs.search() method passes the CIT a variable_types arg by default now

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stewartsiu (so the modification on line 33 should be all we need!)

@stewartsiu
Copy link
Author

Hi @akelleh!

  1. On the print statement, what is your preferred mechanism of displaying info about the independence test? I just added that line when I was debugging to help myself see what it's working on.
  2. On line 33, I didn't change it because I wasn't sure if you intend ChiSquaredTest to be a standalone independence test (I seem to remember some issues with data format) or merely as a helper method for MixedChiSquaredTest.

@akelleh
Copy link
Owner

akelleh commented Apr 21, 2016

Hey @stewartsiu
(1) It's fine to print in general, but I want to avoid adding any more print statements to master, unless they're reporting errors. We want this to be something that doesn't dump a bunch of text to stdout, which is generally bad practice (unless you call a method that specifically asks for text).

(2) That's cool. It'll work as a CIT on discrete data, so we need to make sure it's still compatible with ICs, so we need to make sure it accepts the args that ICs passes to it. Let me know if you have any questions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants