p-value calculations added #2271
Conversation
|
@iglesias I added just p-value calculations to CHAID today. This PR is on that. I have tried to make it as modular as possible. But, unfortunately, its still quite big PR. The methods are all private so I couldn't supply unittests. So, this PR is just for review and cannot be merged until I add at least training method to this which I plan on doing once this part is all set. |
| @@ -180,6 +190,76 @@ class CCHAIDTree : public CTreeMachine<CHAIDTreeNodeData> | |||
| */ | |||
| CLabels* apply_from_current_node(CDenseFeatures<float64_t>* feats, node_t* current); | |||
|
|
|||
| /** calculates adjusted p-value using Bonferroni adjustments | |||
| * | |||
| * @param p-value unadjusted p-value | |||
There was a problem hiding this comment.
Use underscore as in the argument name. Otherwise, Doxygen will report a warning.
|
It is looking all right. Go ahead! |
|
@iglesias I have added tree growing method. I will add to this missing values feature, train_machine method along with unittests tomorrow. Until then this is for review only! |
|
@iglesias I have added the remaining parts of tree growing process along with unittests. Please have a look! |
| /** initializes members of class */ | ||
| void init(); | ||
|
|
||
| public: | ||
| /** denotes that a feature in a vector is missing MISSING = NOT_A_NUMBER */ | ||
| /** denotes that a feature in a vector is missing MISSING = MAX_REAL_NUMBER */ |
There was a problem hiding this comment.
Why going from nan to max float?
|
It is looking good for the moment. See the minor comments (mainly suggestions) above. Although I am in fact curious why you decided to use max float instead of nan for the missing values. |
|
@iglesias I have addressed your previous comments. Please have a look! I will send in apply methods for CHAID as a next PR. |
|
Please rebase locally and update the PR pushing to your branch. GitHub tells me this cannot be automatically merged. |
|
And write a dummy comment here afterwards ;-) |
|
@iglesias Please see if the merging problem is solved. Thanks! |
|
Yep, waiting for travis now. |
Reference