Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AL-1713] Dicom #1572

Merged
merged 96 commits into from
Apr 6, 2022
Merged

[AL-1713] Dicom #1572

merged 96 commits into from
Apr 6, 2022

Conversation

farizrahman4u
Copy link
Contributor

🚀 🚀 Pull Request

Checklist:

  • My code follows the style guidelines of this project and the Contributing document
  • I have commented my code, particularly in hard-to-understand areas
  • I have kept the coverage-rate up
  • I have performed a self-review of my own code and resolved any problems
  • I have checked to ensure there aren't any other open Pull Requests for the same change
  • I have described and made corresponding changes to the relevant documentation
  • New and existing unit tests pass locally with my changes

Changes

@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #1572 (a4566b9) into main (0955356) will decrease coverage by 0.12%.
The diff coverage is 84.55%.

❗ Current head a4566b9 differs from pull request most recent head 7c7aa5d. Consider uploading reports for the commit 7c7aa5d to get more accurate results

@@            Coverage Diff             @@
##             main    #1572      +/-   ##
==========================================
- Coverage   92.47%   92.34%   -0.13%     
==========================================
  Files         201      202       +1     
  Lines       18611    18612       +1     
==========================================
- Hits        17210    17188      -22     
- Misses       1401     1424      +23     
Flag Coverage Δ
unittests 92.34% <84.55%> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
hub/api/read.py 100.00% <ø> (ø)
hub/api/tests/test_api.py 100.00% <ø> (ø)
setup.py 0.00% <ø> (ø)
hub/core/sample.py 84.89% <56.41%> (-5.03%) ⬇️
hub/core/compression.py 87.88% <88.57%> (-0.57%) ⬇️
hub/api/tests/test_dicom.py 100.00% <100.00%> (ø)
hub/api/tests/test_sample_info.py 85.54% <100.00%> (ø)
hub/compression.py 97.36% <100.00%> (+0.07%) ⬆️
hub/core/chunk_engine.py 94.79% <100.00%> (+<0.01%) ⬆️
hub/core/dataset/dataset.py 92.96% <100.00%> (-0.07%) ⬇️
... and 18 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 522b08b...7c7aa5d. Read the comment docs.

Base automatically changed from fr_sample_meta2 to main March 30, 2022 14:23
@tatevikh tatevikh requested review from AbhinavTuli, FayazRahman and davidbuniat and removed request for FayazRahman March 30, 2022 14:40
Copy link
Member

@davidbuniat davidbuniat left a comment

Choose a reason for hiding this comment

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

Few remarks and questions

  • DICOM itself is a container who has multiple types of lossless compression methods currently under cover. What if the user is going to adjust?
  • Do we apply any compression to the metadata in sample_info?
  • Have you tried to train a model on dicom file? (the exercise of doing it will help to truly understand user experience)
    Other than that overall looked good to me.

hub/core/sample.py Outdated Show resolved Hide resolved
@farizrahman4u
Copy link
Contributor Author

farizrahman4u commented Apr 5, 2022

@davidbuniat

Few remarks and questions

  • DICOM itself is a container who has multiple types of lossless compression methods currently under cover. What if the user is going to adjust?
  • Do we apply any compression to the metadata in sample_info?
  • Have you tried to train a model on dicom file? (the exercise of doing it will help to truly understand user experience)
    Other than that overall looked good to me.
  • User can't.
  • Meta data has no compression (for faster query)
  • TODO :)

@farizrahman4u farizrahman4u merged commit bcbf886 into main Apr 6, 2022
@farizrahman4u farizrahman4u deleted the fr_dicom branch April 6, 2022 20:43
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.

None yet

4 participants