Skip to content

Conversation

eddiebergman
Copy link
Contributor

@eddiebergman eddiebergman commented Aug 3, 2022

Fixes the below bug and also provides the X_data with every call to self._loss, which was missing in some parts.


@eddiebergman eddiebergman added this to the V0.15 milestone Aug 3, 2022
@eddiebergman eddiebergman self-assigned this Aug 3, 2022
@eddiebergman eddiebergman requested a review from mfeurer August 3, 2022 14:45
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.

I really like how this PR cuts down bloat. I have left a few minor comments. Could you also please add a unit test?

@eddiebergman
Copy link
Contributor Author

I'm not really sure what to unit test here to be honest, could you be a bit more precise?

@mfeurer
Copy link
Contributor

mfeurer commented Aug 4, 2022

After thinking more about what this PR fixes (making the indexing of pandas dataframes and numpy arrays consistent) we don't need a unit test right now to ensure this - we don't have unit tests for pandas and therefore haven't previously caught this error, and this PR simplifies how we access pandas and we can see from the changelog that it is a clear improvement that right now does not require testing.

@mfeurer
Copy link
Contributor

mfeurer commented Aug 4, 2022

There are also unit tests failing wrt the changes (they are hidden among the other failiing unit tests, so you could maybe rebase on development where they are fixed?)?

@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #1550 (c4b2c2b) into development (9035f05) will increase coverage by 0.22%.
The diff coverage is 98.38%.

@@               Coverage Diff               @@
##           development    #1550      +/-   ##
===============================================
+ Coverage        84.43%   84.66%   +0.22%     
===============================================
  Files              157      157              
  Lines            11948    11976      +28     
  Branches          2063     2065       +2     
===============================================
+ Hits             10088    10139      +51     
+ Misses            1297     1286      -11     
+ Partials           563      551      -12     

Impacted file tree graph

@eddiebergman eddiebergman force-pushed the fix-1547-Bug-in-_partial_fit_and_predict_iterative-in-train_evaluator.py_ branch from 1351dd2 to 9769082 Compare August 4, 2022 14:19
@eddiebergman eddiebergman force-pushed the fix-1547-Bug-in-_partial_fit_and_predict_iterative-in-train_evaluator.py_ branch from 9769082 to 4ab8dc6 Compare August 5, 2022 07:52
@eddiebergman
Copy link
Contributor Author

Seems the meta learning timeouts have struck again ... I really don't know why, going to rerun the tests

@mfeurer mfeurer merged commit 8efe1fd into development Aug 8, 2022
@mfeurer mfeurer deleted the fix-1547-Bug-in-_partial_fit_and_predict_iterative-in-train_evaluator.py_ branch August 8, 2022 16:40
github-actions bot pushed a commit that referenced this pull request Aug 8, 2022
eddiebergman added a commit that referenced this pull request Aug 18, 2022
….py_ (#1550)

* Create PR

* Fix issue with incorrect splits and data

* Remove extraneous indexing

* Fill in missing points of X_data to self._loss

* Fix typevar

* Fix X data passed in to loss

* Fix indexing of X_fold for train evalutor

* Fix cv-iterative-fit successive halving example
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.

Bug in _partial_fit_and_predict_iterative in train_evaluator.py?

2 participants