kegl commented 3 years ago

This is an open PR to see the status of the advanced branch wrt master. The CDS team @agramfort @glemaitre decides if it is merged.

Additions compared to master:

[x] Decoupled CV #243
[x] --data-label option to specify a dataset when running ramp-test #245
[x] fixing ramp-leaderboard (including fix for --bagged and --data-label option) #247
[x] adding an option to ramp-blend to select the --score_metrics_index to blend on #247

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging 7fbae0128537c2d15d89f4e056eb429681a0d6dd into b92ca38287c94124444f51f41d9231d0a94db3cc - view on LGTM.com

fixed alerts:

1 for Unreachable code

codecov[bot] commented 3 years ago

Codecov Report

Merging #246 (da3bdd2) into master (cbc3e83) will decrease coverage by 0.44%. The diff coverage is 25.00%.

:exclamation: Current head da3bdd2 differs from pull request most recent head ccca466. Consider uploading reports for the commit ccca466 to get more accurate results

@@            Coverage Diff             @@
##           master     #246      +/-   ##
==========================================
- Coverage   82.94%   82.49%   -0.45%     
==========================================
  Files         137      137              
  Lines        4684     4678       -6     
==========================================
- Hits         3885     3859      -26     
- Misses        799      819      +20

Impacted Files	Coverage Δ
rampwf/utils/__init__.py	`100.00% <ø> (ø)`
rampwf/utils/cli/blend.py	`0.00% <0.00%> (-64.52%)`	:arrow_down:
rampwf/utils/cli/testing.py	`62.50% <0.00%> (ø)`
rampwf/utils/testing.py	`100.00% <ø> (ø)`
rampwf/utils/cli/show.py	`76.40% <73.33%> (ø)`
rampwf/utils/cli/tests/test_cli.py	`96.72% <100.00%> (-0.30%)`	:arrow_down:

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 593b9fb...ccca466. Read the comment docs.

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging b7cdedd99be982586039c2ff1cd92a34225c7302 into cec16a02e04fa1aea353286fef393231c967e2ef - view on LGTM.com

fixed alerts:

1 for Unreachable code

lgtm-com[bot] commented 3 years ago

This pull request fixes 1 alert when merging 864906d10b6fcf088a84fc2d64644dcb782190a9 into cec16a02e04fa1aea353286fef393231c967e2ef - view on LGTM.com

fixed alerts:

1 for Unreachable code

lgtm-com[bot] commented 3 years ago

This pull request fixes 9 alerts when merging f9b1939b0ef0db32f291c80a2a9313e3c2176ea6 into f1c78ea5acc484ccef8c7986138717a7b7420f2c - view on LGTM.com

fixed alerts:

5 for Implicit string concatenation in a list
2 for Module is imported with 'import' and 'import from'
1 for Module is imported more than once
1 for Unreachable code

lgtm-com[bot] commented 3 years ago

This pull request fixes 9 alerts when merging 3eadb4dace8d003d465492bacefc4f8a5a68aae4 into f990277607cb3f23ac68f783003821761843d686 - view on LGTM.com

fixed alerts:

5 for Implicit string concatenation in a list
2 for Module is imported with 'import' and 'import from'
1 for Module is imported more than once
1 for Unreachable code

lgtm-com[bot] commented 3 years ago

This pull request fixes 9 alerts when merging 96f6efb79ab68425c0aaa31f973d1992ed7407fc into f990277607cb3f23ac68f783003821761843d686 - view on LGTM.com

fixed alerts:

5 for Implicit string concatenation in a list
2 for Module is imported with 'import' and 'import from'
1 for Module is imported more than once
1 for Unreachable code

lgtm-com[bot] commented 3 years ago

This pull request fixes 9 alerts when merging 45cce460e50690a8ffe0c38f69c9df655d5dd78a into f990277607cb3f23ac68f783003821761843d686 - view on LGTM.com

fixed alerts:

5 for Implicit string concatenation in a list
2 for Module is imported with 'import' and 'import from'
1 for Module is imported more than once
1 for Unreachable code

lgtm-com[bot] commented 3 years ago

This pull request fixes 9 alerts when merging 1676d9d878faab1965084dc285cbc45261c67898 into 67a26b3e70c87fd2bf0cea3c893e87c11739c634 - view on LGTM.com

fixed alerts:

5 for Implicit string concatenation in a list
2 for Module is imported with 'import' and 'import from'
1 for Module is imported more than once
1 for Unreachable code

lgtm-com[bot] commented 3 years ago

This pull request fixes 9 alerts when merging ec8835ef68de2389b54e7e730f1967a17b6100db into 67a26b3e70c87fd2bf0cea3c893e87c11739c634 - view on LGTM.com

fixed alerts:

5 for Implicit string concatenation in a list
2 for Module is imported with 'import' and 'import from'
1 for Module is imported more than once
1 for Unreachable code

kegl commented 3 years ago

I'd be favorable to merge this (and the corresponding branch on ramp-board) into master after the data camp season. We have now a second server running the advanced branch seamlessly, with one challenge finished. We also have a protocol put in place for testing both workflow and board before merging anything into workflow (not automatic but easy to follow): https://github.com/paris-saclay-cds/ramp-workflow/tree/advanced#testing-ramp-board

The advantage would be to unify our workforces for maintenance and development.

@agramfort was the main opponent to this, I'll let you decide.

agramfort commented 3 years ago

yes datacamp season is over. server should be quiet for the next 3-4 weeks.

@rth can you take care of this? merge, make a release and update the prod server?

agramfort commented 3 years ago

note that this cannot be merged as is as it refers to instructions that need to be adjusted once this is merged

kegl commented 3 years ago

@agramfort , can you elaborate? You mean the testing instructions here https://github.com/paris-saclay-cds/ramp-workflow/tree/advanced#testing-ramp-board? What's your suggestion, would you like to first PR the README?

The issue is that we're close to launching the permanent Huawei site, where there will be development the make the site more pro (accessibility, GDPR), which may also be interesting for ramp.studio. It would be useful to merge the two branches before that.

rth commented 3 years ago

I think Alex meant that say doc/index.rst currently specifies to install this branch (and some CI changes) that would need to be updated before merging (and possibly other updated parts of the doc).

merge, make a release and update the prod server?

Yes, I can help with that next week. It would indeed be great to keep a common code base.

kegl commented 3 years ago

OK yes, got it :), thx @rth

lgtm-com[bot] commented 3 years ago

This pull request fixes 9 alerts when merging 74686f73391191e42d25cb0eb7d11d2f53571a1c into e97a27235a8dbd68111ca6b0c9136ff35cab81f8 - view on LGTM.com

fixed alerts:

5 for Implicit string concatenation in a list
2 for Module is imported with 'import' and 'import from'
1 for Module is imported more than once
1 for Unreachable code

rth commented 3 years ago

All changes from this branch were merged as separate PRs into master, so I'm going to close this PR.

albertcthomas commented 3 years ago

Thanks a lot @rth

paris-saclay-cds / ramp-workflow

Advanced #246

Codecov Report