Skip to content

Conversation

@franchuterivera
Copy link
Contributor

  • Creates fit/transform interface to input validator
  • Adds more checking thanks to pytest (we also move to pytest instead of unittest)

@codecov
Copy link

codecov bot commented Dec 20, 2020

Codecov Report

Merging #1041 (3645162) into development (4d19551) will increase coverage by 0.20%.
The diff coverage is 95.49%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1041      +/-   ##
===============================================
+ Coverage        85.46%   85.66%   +0.20%     
===============================================
  Files              127      128       +1     
  Lines            10177    10272      +95     
===============================================
+ Hits              8698     8800     +102     
+ Misses            1479     1472       -7     
Impacted Files Coverage Δ
autosklearn/automl.py 84.30% <83.33%> (-0.40%) ⬇️
autosklearn/data/feature_validator.py 96.35% <96.35%> (ø)
autosklearn/data/target_validator.py 96.96% <96.96%> (ø)
autosklearn/data/validation.py 97.14% <97.05%> (+7.74%) ⬆️
...ature_preprocessing/select_rates_classification.py 83.09% <0.00%> (-4.23%) ⬇️
...ine/components/classification/gradient_boosting.py 91.30% <0.00%> (-0.87%) ⬇️
...eline/components/feature_preprocessing/fast_ica.py 93.47% <0.00%> (+2.17%) ⬆️
...ipeline/components/regression/gradient_boosting.py 92.30% <0.00%> (+2.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d19551...3645162. Read the comment docs.

Copy link
Contributor

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

Hi, This looks great but there are a lot of changes to look at, so I only left a few initial comments and remarks and will have a detailed look at the new transformers afterwards.

@mfeurer
Copy link
Contributor

mfeurer commented Jan 5, 2021

I was also wondering whether score works for all metrics, especially those which require probabilities and also binary cases? But maybe this is not within the scope of this PR and needs to be considered in a separate PR?

Copy link
Contributor

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

Part 2 of the review. Tests are still to come.

Copy link
Contributor

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

And one more batch of comments.

@franchuterivera
Copy link
Contributor Author

I was also wondering whether score works for all metrics, especially those which require probabilities and also binary cases? But maybe this is not within the scope of this PR and needs to be considered in a separate PR?

Do you think you can elaborate more on this? Do you mean making sure all metrics works for all type of data?

@franchuterivera
Copy link
Contributor Author

There is one thing pending that needs further discussion... which is the fact that infer objects from pandas will not make an object column with letters categorical.

Should we handle this ourselves using some heuristic?

@mfeurer
Copy link
Contributor

mfeurer commented Jan 8, 2021

Do you think you can elaborate more on this? Do you mean making sure all metrics works for all type of data?

Yes, that's what I meant here. The score code looks a bit broken to me at the moment.

There is one thing pending that needs further discussion... which is the fact that infer objects from pandas will not make an object column with letters categorical.

As I mentioned above, this will be handled by an upcoming PR.

@mfeurer mfeurer merged commit a615c3e into automl:development Jan 12, 2021
github-actions bot pushed a commit that referenced this pull request Jan 12, 2021
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.

2 participants