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

Update URLs in tests #36

Merged
merged 4 commits into from
Oct 14, 2019
Merged

Update URLs in tests #36

merged 4 commits into from
Oct 14, 2019

Conversation

notZaki
Copy link
Member

@notZaki notZaki commented Sep 6, 2019

The tests have been failing recently because they need to download data but those URLs no longer work.
This PR updates the outdated URLs.

Other changes:

  • Replaced gunzip with gzip
    • Reason: I had an easier time setting up gzip to work on windows, whereas I couldn't figure out how to install gunzip
  • Removed Julia 0.7 from the travis tests since it is similar to 1.0. Added Julia 1.2 for completeness.

@codecov
Copy link

codecov bot commented Sep 6, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ba85142). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #36   +/-   ##
=========================================
  Coverage          ?   87.55%           
=========================================
  Files             ?        1           
  Lines             ?      249           
  Branches          ?        0           
=========================================
  Hits              ?      218           
  Misses            ?       31           
  Partials          ?        0

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 ba85142...ca713d3. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 6, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ba85142). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #36   +/-   ##
=========================================
  Coverage          ?   87.55%           
=========================================
  Files             ?        1           
  Lines             ?      249           
  Branches          ?        0           
=========================================
  Hits              ?      218           
  Misses            ?       31           
  Partials          ?        0

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 ba85142...766c594. Read the comment docs.

@sjkelly
Copy link

sjkelly commented Sep 7, 2019

GZip change looks good.

We might just want to host the test files on GitHub or somewhere else. I think if DropBox gets weird requests it will block them.

@Tokazama
Copy link
Member

Tokazama commented Sep 7, 2019

It might be worth pulling some files from here (look under "Slice timing correction" for the first actual DICOM file mention). This would probably be more ambitious than what you're going for now

@sjkelly
Copy link

sjkelly commented Sep 7, 2019

@notZaki
Copy link
Member Author

notZaki commented Sep 11, 2019

Thanks for the links with the DICOM samples @sjkelly @Tokazama .
I've been wanting to extend the tests to include some files from the dcm_qa repositories mentioned in another issue but haven't had the time to look through them.

The current tests use a few files from Barré's DICOM samples. That website was recently updated and the sample files are now on dropbox.
If dropbox is an issue, then the DICOM samples could be checked into this repository (adds a bit of bloat #22) or hosted in a separate repository.

I'm leaning towards the latter option of hosting them in a separate repository and I'll try doing that this week. Let me know if there's any other suggestions.

We can extend the tests as well with more DICOM samples, but I would prefer getting the current tests dealt with first.

@Tokazama
Copy link
Member

Fixing what we have now and extending later sounds very reasonable.

@sjkelly
Copy link

sjkelly commented Sep 20, 2019

Sounds good. As an aside I have been working to make isosurface extraction work better with NRRD and DICOMs in Meshing.jl. I will add a DICOM example here soon: https://juliageometry.github.io/Meshing.jl/dev/examples/#NRRD-Data-1

@notZaki
Copy link
Member Author

notZaki commented Sep 20, 2019

Sounds good @sjkelly

On my end, I made a github repository containing the DICOM files for testing.
The testing code was refactored a bit since we no longer have to worry about g/unzipping---this also makes it easier to test on windows.

@notZaki
Copy link
Member Author

notZaki commented Oct 7, 2019

If there are no objections then I'll merge this by the end of the week.

@notZaki notZaki merged commit 39045c5 into JuliaHealth:master Oct 14, 2019
@notZaki notZaki deleted the updateTestURL branch October 14, 2019 02:47
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.

3 participants