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

Add PNG microscopy data and files for testing dataframe #6

Merged
merged 9 commits into from
Dec 21, 2020

Conversation

mariehbourget
Copy link
Member

Related to #5
This PR aims to add microscopy data to the data-testing repo.
It also covers the _sessions.tsv and _scans-tsv files and architecture needed for the testing of the new loader in ivadomed.

Currently, I have an issue with the data that was already in the repo (MRI anat), because it is used for testing in ivadomed and the path is harcoded in several tests and config files:

  • Ideally, we would want to have different BIDS dataset folders inside the data-testing repo (e.g. anat, microscopy, eeg, dwi, etc).
  • So I added a microscopy BIDS folders for PNG format in microscopy_png and updated the README accordingly.
  • However, files related to MRI anat are still at the root of data-testing (all tests passed).
  • If I move them into a "mri-anat" dataset folder for example, 48 tests are failing because of the paths.

I would like to go ahead and write a test for the new loader. I can do it even if the data stays like that, but it is not very clean and I think it would be best to update the MRI anat data as well (in another PR) @lrouhier @charleygros what do you think? Thanks!

@mariehbourget mariehbourget marked this pull request as draft December 14, 2020 23:32
@lrouhier
Copy link
Contributor

I think You are right this is not very clean, and even though 48 seems like a big number I don't think it would take too much time to fix (because this would only be a path_related issue). I don't know about the order though, should we break everything now or wait for the JOSS review to be over (because we will do a release after the review right ?)

@mariehbourget
Copy link
Member Author

I don't know about the order though, should we break everything now or wait for the JOSS review to be over (because we will do a release after the review right ?)

I think we should wait for the JOSS review, I'll write the tests with the current folder hierarchy of this branch and I'll fix it afterwards.

@charleygros
Copy link
Member

I think we should wait for the JOSS review, I'll write the tests with the current folder hierarchy of this branch and I'll fix it afterwards.

Ideally we should wait for the JOSS review. But we don't really know when it will finish (only one reviewer left tho): I don't want to freeze your development for an uncertain period of time :/

@mariehbourget mariehbourget changed the title Add microscopy data Add microscopy data and files for testing dataframe Dec 16, 2020
@mariehbourget mariehbourget changed the title Add microscopy data and files for testing dataframe Add PNG microscopy data and files for testing dataframe Dec 16, 2020
@mariehbourget
Copy link
Member Author

Ideally we should wait for the JOSS review. But we don't really know when it will finish (only one reviewer left tho): I don't want to freeze your development for an uncertain period of time :/

I wrote the tests for the function with the file architecture of this branch in PR #584 in ivadomed repo.
All previous tests are still passing on ivadomed.

Here is what I suggest:

  1. Merge this PR
  2. Create a new release for data-testing
  3. Update the link in download_data in PR #584 in ivadomed here to point to the new data-testing release
  4. Merge PR 584 in ivadomed (all tests should still passed)
  5. Wait for JOSS review
  6. Open a new PR here to update files architecture, and a new PR on ivadomed to fix paths in tests.

If we want to be hyper-cautious, we can wait for JOSS review before step 4. This won't freeze the development on my part.

@mariehbourget mariehbourget marked this pull request as ready for review December 16, 2020 15:40
@jcohenadad
Copy link
Member

If we want to be hyper-cautious, we can wait for JOSS review before step 4. This won't freeze the development on my part.

I think it's fine. Worst case scenario: there is a glitch for a few hours, while one reviewer is looking at the repos, but they would easily understand it's a temporary issue that will be fixed a few hours later. If it glitches happen to Google it can happen to us 😉

@mariehbourget
Copy link
Member Author

This PR is ready for review.
After merged, a new release must be done in order to update the download link in #584 (ivadomed).
@charleygros or @andreanne-lemay, would you like to review?

Copy link
Member

@andreanne-lemay andreanne-lemay left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

microscopy_png/participants.tsv Show resolved Hide resolved
@mariehbourget
Copy link
Member Author

Thank you @andreanne-lemay and @charleygros, I'll merge and create the new release.

@mariehbourget mariehbourget merged commit 842c277 into master Dec 21, 2020
@mariehbourget mariehbourget deleted the mhb/add-microscopy-data branch December 21, 2020 22:59
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.

5 participants