search

X-DataInitiative / SCALPEL-Extraction

SCALPEL extraction library to fetch concepts from flattened SNDS
BSD 3-Clause "New" or "Revised" License
17 stars 3 forks source link

Cnam 322 refactor mlpp loader #140

Closed dsun0720 closed 6 years ago

dsun0720 commented 6 years ago

1: refactor the MLPP Loader 2: add the test units 3: format

danielpes commented 6 years ago

Please check your tests.

codecov[bot] commented 6 years ago

Codecov Report

Merging #140 into master will decrease coverage by 0.82%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
- Coverage   94.39%   93.56%   -0.83%     
==========================================
  Files         121      125       +4     
  Lines        1464     1493      +29     
  Branches       52       36      -16     
==========================================
+ Hits         1382     1397      +15     
- Misses         82       96      +14
Impacted Files Coverage Δ
...map/cnam/etl/filters/PatientFiltersImplicits.scala 97.29% <ø> (ø) :arrow_up:
...l/transformers/follow_up/FollowUpTransformer.scala 100% <ø> (ø) :arrow_up:
...nique/cmap/cnam/etl/loaders/mlpp/MLPPFeature.scala 100% <ø> (ø) :arrow_up:
.../transformers/exposures/ExposuresTransformer.scala 100% <ø> (ø) :arrow_up:
...r/polytechnique/cmap/cnam/util/RichDataFrame.scala 100% <100%> (ø)
...hnique/cmap/cnam/etl/loaders/mlpp/MLPPLoader.scala 100% <100%> (ø) :arrow_up:
...ique/cmap/cnam/etl/loaders/mlpp/MLPPOutcomes.scala 100% <100%> (ø)
.../cmap/cnam/etl/loaders/mlpp/DiscreteExposure.scala 100% <100%> (ø)
...que/cmap/cnam/etl/loaders/mlpp/MLPPExposures.scala 100% <100%> (ø)
...que/cmap/cnam/etl/loaders/mlpp/MLPPDataFrame.scala 100% <100%> (ø)
... and 7 more

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 fd5c8a2...9e8799c. Read the comment docs.

dsun0720 commented 6 years ago

@firas16 we took 4 implicit conversions out of MLPPLoader, so i put related tests into 4 test units: DiscreteExposureImplicitsSuite, MLPPDataFrameImplicitsSuite, MLPPOutcomesImplicitsSuite, MLPPExposuresImplicitsSuite

danielpes commented 6 years ago

@dsun0720 Could you perform a partial rebase starting from commit 4f8d5b on top of master and resolve the conflicts? I'll do my review after that. Tell us if you need any help and make sure everything is correct before force pushing.

danielpes commented 6 years ago

Don't forget to rebase, squash and check if everything is allright before merging.

dsun0720 commented 6 years ago

the outputRootPath will be rebuilt in the CNAM-327 with pureconfig