Closed kegl closed 4 years ago
This pull request fixes 1 alert when merging b004ad3c7b59f31cbf1b862596e27e88ac9c6de8 into b92ca38287c94124444f51f41d9231d0a94db3cc - view on LGTM.com
fixed alerts:
@kegl do you anticipate any issue with ramp-board when updating rampwf based on this PR?
Merging #243 into advanced will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## advanced #243 +/- ##
=========================================
Coverage 81.75% 81.75%
=========================================
Files 133 133
Lines 4921 4921
=========================================
Hits 4023 4023
Misses 898 898
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 b92ca38...ade4748. Read the comment docs.
@agramfort no, the interface remained intact.
The only issue is if a kit implemented its own Predictions type: in this case __init__
needs to be changed. AFAIK there is no such kit, but I think the best tool for this would be https://travis-ci.org/github/paris-saclay-cds/rampwf-kits-test-master: all kits that we want to maintain should be there.
This pull request fixes 1 alert when merging ee64524102fc4e4bf2f4362962299155625f22e7 into b92ca38287c94124444f51f41d9231d0a94db3cc - view on LGTM.com
fixed alerts:
This pull request fixes 1 alert when merging 8d7bba6f7050c17ac87fff1f52a709cdda3fb979 into b92ca38287c94124444f51f41d9231d0a94db3cc - view on LGTM.com
fixed alerts:
While editing scoring.rst, I corrected a small mistake: the public leaderboard displays the bagged validation score, not the mean. I checked it on the new ramp.studio site, it still seems to be the case.
if there is a bug on master can you fix it in a dedicated PR? thanks
It was a mistake in the documentation (not in the code) which I edited anyway as part of documenting the data flow.
@agramfort is it OK if @albertcthomas and/or @gabriel-hurtado reviews this or would you like someone also from the CDS side?
I just want to guarantee that it will not break the ramp.studio website if we use what comes out of this PR.
Yes, I agree. How can we check that? Can we do a branch on ramp-board where we change travis and pull ramp-workflow from this branch instead of master?
Maybe all it would require is to install ramp-workflow from https://github.com/kegl/ramp-workflow/tree/decoupled_cv instead of using pip, here:
https://github.com/paris-saclay-cds/ramp-board/blob/master/ci_tools/environment_iris_kit.yml
@agramfort would my previous suggestion assure you? If yes, could pls @maikia do it, I'm not very skillful at scripting the new test. If not, what would be your suggestion? How did you test previous updates to ramp-workflow?
I added some scripts to the doc that can be executed line by line e.g. in a notebook, e.g for debugging a RAMP kit. Furthermore it can also play a pedagogic role to show ML students what single and double cross validation are. cc @agramfort https://github.com/paris-saclay-cds/ramp-workflow/blob/9d9ccedf81286cb2d85c9415c2900e1639df3bfd/doc/scoring.rst
This pull request fixes 1 alert when merging 9d9ccedf81286cb2d85c9415c2900e1639df3bfd into b92ca38287c94124444f51f41d9231d0a94db3cc - view on LGTM.com
fixed alerts:
This pull request fixes 1 alert when merging b94a4291f4affdeb69d7f9d7de03d1d67e947904 into b92ca38287c94124444f51f41d9231d0a94db3cc - view on LGTM.com
fixed alerts:
I added the general test protocol to test ramp-board with the PR'd ramp-workflow branch to https://github.com/kegl/ramp-workflow/blob/decoupled_cv/README.rst. I followed the protocol to test this PR on ramp-board. There was an issue since ramp-board does not completely rely on the ramp-workflow scoring. I fixed it and made all tests green. Big shout-out to @glemaitre and @jorisvandenbossche for writing the ramp-board tests, without those these kinds of changes would have been very dangerous. Thanks!
I think now the PR can be merged, let me know @agramfort if we can go ahead.
This pull request fixes 1 alert when merging 7e6b448d4f50dc4b9f7662f9b0b2842a60aeec9b into b92ca38287c94124444f51f41d9231d0a94db3cc - view on LGTM.com
fixed alerts:
This pull request fixes 1 alert when merging ade4748947e9f35803879753457760fc462fe274 into b92ca38287c94124444f51f41d9231d0a94db3cc - view on LGTM.com
fixed alerts:
This is to close #184.
test_submission