neurodata / treeple

Scikit-learn compatible decision trees beyond those offered in scikit-learn
https://treeple.ai
Other
65 stars 14 forks source link

DOC update tutorial figure & format & ENH optimize p value calculation #273

Closed PSSF23 closed 5 months ago

PSSF23 commented 6 months ago
adam2392 commented 6 months ago

For the sake of code-review culture:

minor changes for figure color and code block formats

What do the changes induce? And why are they needed? Perhaps a brief description in the GH PR, or in the in-line comments.

PSSF23 commented 6 months ago

For the sake of code-review culture:

minor changes for figure color and code block formats

What do the changes induce? And why are they needed? Perhaps a brief description in the GH PR, or in the in-line comments.

Reverse color for class zero and one, Add the missing && signs in the rst paragraphs.

adam2392 commented 5 months ago

No idea where the python CI error is from.

I updated your branch wrt main. The only remaining CI errors should be from Mac-osX Python3.9/3.10 builds, which should get fixed upstream soon(?) https://github.com/actions/setup-python/issues/855

If there are other CI errors, it possibly is related to your PR code changes, but we can wait and see what the CI results are.

adam2392 commented 5 months ago

Perhaps update your spin locally and see if https://github.com/neurodata/scikit-tree/blob/main/.spin/cmds.py needs to get modified at all? It's possible based on the obscure message seen in the CIs.

adam2392 commented 5 months ago

Wanna also take a stab at quick fixing the CIs related to macos images: https://github.com/actions/setup-python/issues/855#issuecomment-2096792205

adam2392 commented 5 months ago

We'll definitely keep the meson builds as that's the modern way of building packages with python and c++.

adam2392 commented 5 months ago

Rn i can't fix the meson issues but it's something you can test locally by running the same command to enable coverage to see if you can replicate. Then I mentioned a few things you could look into.

For now, I don't think we can merge in changes because the failing CI are related to unit testing. We don't have the CI guarantee, so best practice would be to try to address the issue. Otw we might have to backtrack and fix it later on.

I can circle back if you don't have time after NeurIPS deadline.

adam2392 commented 5 months ago

I agree! I just meant I'd rather err on side of caution cuz the CI rn is unable to run the test suite at all. So a test could fail on one of the machines because of some obscure but meaningful reason and we have no insight.

If we merge it in, and unit tests on linux for some reason raised an issue later on when the CI build errors are fixed someone will have to come back, spend time and figure out why the unit tests failed and what LOC led to that failure.

I have done that in the past which incurs significant time on my part, so just want to see if we can hold off until someone has a Sep PR to address the CI issues. That way we are at least 100% sure the CI unit tests suggest the PR changes are fine.

I can commit to having that Sep PR but just can't do it until end of next week. In the meantime, I can point you in right direction, if you want to fix the issue beforehand. Does this sound okay with you?

adam2392 commented 5 months ago

TLDR: your PR LGTM! Just want to wait on merging till we can rerun the CI.

sampan501 commented 5 months ago

@adam2392 we are thinking of sending tutorials to Exact soon, so I suggest that we merge this PR as it mostly just changes docs files and builds correctly on most other operating systems

adam2392 commented 5 months ago

Sure does someone want to try fixing the CI?

sampan501 commented 5 months ago

Fixed in #279