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

Add random forest classifier for tomographer #1

Merged
merged 14 commits into from
Sep 11, 2023
Merged

Conversation

hangqianjun
Copy link
Contributor

@hangqianjun hangqianjun commented Jul 3, 2023

Change Description

Solution Description

Moved random forest classifier for the tomographer class from rail_base to rail_sklearn due to its dependence on sklearn.

Code Quality

  • I have read the Contribution Guide
  • My code follows the code style of this project
  • My code builds (or compiles) cleanly without any errors or warnings
  • My code contains relevant comments and necessary documentation

Project-Specific Pull Request Checklists

Bug Fix Checklist

  • My fix includes a new test that breaks as a result of the bug (if possible)
  • My change includes a breaking change
    • My change includes backwards compatibility and deprecation warnings (if possible)

New Feature Checklist

  • I have added or updated the docstrings associated with my feature using the NumPy docstring format
  • I have updated the tutorial to highlight my new feature (if appropriate)
  • I have added unit/End-to-End (E2E) test cases to cover my new feature
  • My change includes a breaking change
    • My change includes backwards compatibility and deprecation warnings (if possible)

Documentation Change Checklist

Build/CI Change Checklist

  • If required or optional dependencies have changed (including version numbers), I have updated the README to reflect this
  • If this is a new CI setup, I have added the associated badge to the README

Other Change Checklist

  • Any new or updated docstrings use the NumPy docstring format.
  • I have updated the tutorial to highlight my new feature (if appropriate)
  • I have added unit/End-to-End (E2E) test cases to cover any changes
  • My change includes a breaking change
    • My change includes backwards compatibility and deprecation warnings (if possible)

@codecov
Copy link

codecov bot commented Jul 3, 2023

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@hangqianjun
Copy link
Contributor Author

hangqianjun commented Aug 4, 2023

Unit tests pass on local machine but not on GitHub. Waiting for new release of rail to include classifier class.

Copy link
Contributor

@aimalz aimalz left a comment

Choose a reason for hiding this comment

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

I requested some minor changes to the class names for consistency and made the suggestion to isolate the color-making function in this module to make it easy to swap out when we unify all the calculations of colors that appear in so many of the algos modules.

src/rail/estimation/algos/random_forest.py Outdated Show resolved Hide resolved
src/rail/estimation/algos/random_forest.py Show resolved Hide resolved
tests/sklearn/test_algos.py Outdated Show resolved Hide resolved
tests/sklearn/test_algos.py Outdated Show resolved Hide resolved
name = 'randomForestClassifier'
config_options = CatClassifier.config_options.copy()
config_options.update(
id_name=Param(str, "", msg="Column name for the object ID in the input data, if empty the row index is used as the ID."),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing needs to be changed here, but I just want to note re: LSSTDESC/rail_base#39 that this will be the first instance of IDs in the RAIL-iverse so should be considered in decision-making about how to consistently reference these throughout src/rail code.

Copy link
Contributor

@aimalz aimalz left a comment

Choose a reason for hiding this comment

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

Looks good! This can be merged as soon as the tests are passing.

@eacharles eacharles merged commit 10ba2ab into main Sep 11, 2023
8 checks passed
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