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

fix: adding random forest classifier test which breaks #301

Merged
merged 18 commits into from
Apr 12, 2024
Merged

fix: adding random forest classifier test which breaks #301

merged 18 commits into from
Apr 12, 2024

Conversation

mmann1123
Copy link
Collaborator

I identified an untested bug in geowombat. Basically sklearn-xarray needs scikit-learn=1.2.0 or it breaks with forest estimators. Adding test and trying to limit scikit version in setup.cfg for ml add-on install.
had to set scikit-learn to 1.2.0 to fix the issue

What is this PR changing?

Add a description

Checklist

  • [ x] Remember to add a semantic tag to the commit name

Tag options:

  • feat: A new feature
  • fix: A bug fix
  • docs: Documentation only changes
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • refactor: A code change that neither fixes a bug nor adds a feature
  • perf: A code change that improves performance
  • test: Adding missing or correcting existing tests
  • chore: Changes to the build process or auxiliary tools and libraries such as documentation generation

Example:

fix: <branch name> PR number

@mmann1123 mmann1123 added bug Something isn't working dependencies Pull requests that update a dependency file labels Mar 28, 2024
@mmann1123 mmann1123 requested a review from jgrss March 28, 2024 03:17
@mmann1123
Copy link
Collaborator Author

@jgrss after a bit of mussing about this is ready to look at

@mmann1123 mmann1123 self-assigned this Mar 30, 2024
self.assertTrue(np.all(np.isnan(y2.values[0, 0:5, 0])))
self.assertTrue(
np.allclose(
y1.values[0, -5:-1, 0],
Copy link
Owner

Choose a reason for hiding this comment

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

Curious what's going on here with the indexing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just grabbing some portion of the values to compare. I guess I really don't need to do that but oh well.

@mmann1123
Copy link
Collaborator Author

@jgrss I am not sure what the "Tests - waiting for status to be reported" is. What's the plan for merging here?

@jgrss
Copy link
Owner

jgrss commented Apr 12, 2024

@jgrss I am not sure what the "Tests - waiting for status to be reported" is. What's the plan for merging here?

I can merge it.

@jgrss jgrss merged commit 888eb7b into main Apr 12, 2024
3 checks passed
@fer-at-sea
Copy link

thanks for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants