Skip to content

Conversation

@frank-dong-ms-zz
Copy link
Contributor

@frank-dong-ms-zz frank-dong-ms-zz commented Jul 7, 2020

issue #4909

@frank-dong-ms-zz frank-dong-ms-zz requested a review from a team as a code owner July 7, 2020 23:31
@frank-dong-ms-zz
Copy link
Contributor Author

Below is the original comments by Yael:
The expected behavior should be to either cache the data before fitting the first estimator in the chain, or disallow calling AppendCacheCheckpoint on empty chains, so that the user knows to cache the data explicitly before fitting. (I think the first solution is preferable, but open to other opinions).

I agree we should change current behavior of do nothing when user append cache checkpoint on empty estimator chain as this behavior can be confusion sometime. I would prefer to throw exception when calling AppendCacheCheckpoint on empty estimator chains as this behavior is more clear then apply an cache automatically on chain before first estimator fitting (in my point of view, AppendCacheCheckpoint should be precise and enable cache on the point we call this method, better not to anticipate anything and let user have full control, if user is doing something wrong we should let user know where he/she is doing wrong immediately) but I'm open to any suggestions.

@codecov
Copy link

codecov bot commented Jul 8, 2020

Codecov Report

Merging #5291 into master will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff           @@
##           master    #5291   +/-   ##
=======================================
  Coverage   73.75%   73.76%           
=======================================
  Files        1022     1022           
  Lines      190340   190353   +13     
  Branches    20471    20472    +1     
=======================================
+ Hits       140391   140405   +14     
  Misses      44432    44432           
+ Partials     5517     5516    -1     
Flag Coverage Δ
#Debug 73.76% <85.71%> (+<0.01%) ⬆️
#production 69.52% <66.66%> (+<0.01%) ⬆️
#test 87.62% <90.90%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
...c/Microsoft.ML.Data/DataLoadSave/EstimatorChain.cs 90.00% <66.66%> (+0.34%) ⬆️
test/Microsoft.ML.Tests/CachingTests.cs 97.91% <90.90%> (-2.09%) ⬇️
src/Microsoft.ML.FastTree/Training/StepSearch.cs 57.42% <0.00%> (-4.96%) ⬇️
...icrosoft.ML.AutoML/Experiment/SuggestedPipeline.cs 88.65% <0.00%> (-4.13%) ⬇️
....ML.AutoML/PipelineSuggesters/PipelineSuggester.cs 83.19% <0.00%> (+2.52%) ⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs 100.00% <0.00%> (+20.51%) ⬆️

Copy link
Contributor

@harishsk harishsk left a comment

Choose a reason for hiding this comment

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

:shipit:

@frank-dong-ms-zz frank-dong-ms-zz merged commit bc10d60 into dotnet:master Jul 9, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Mar 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants