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

Create Map using a Dask array #2689

Merged
merged 5 commits into from Jul 18, 2018

Conversation

Projects
None yet
4 participants
@wtbarnes
Copy link
Member

commented Jul 15, 2018

See issue #2687. This PR makes a change to the map factory such that, instead of checking that the input data array is a numpy array, it checks a whitelist of allowed data array types. Currently, these include

  • np.ndarray
  • dask.array.Array

The dependency on dask is still optional. However, it should be noted that scikit image, which is already a sunpy dependency, depends on dask-core so we really get this dependency for free.

I've also included a map factory test.

@pep8speaks

This comment has been minimized.

Copy link

commented Jul 15, 2018

Hello @wtbarnes! Thanks for updating the PR.

Line 101:101: E501 line too long (106 > 100 characters)
Line 105:101: E501 line too long (116 > 100 characters)
Line 177:17: E129 visually indented line with same indent as next logical line
Line 283:13: E722 do not use bare except'
Line 334:5: E722 do not use bare except'

Line 27:1: E265 block comment should start with '# '
Line 29:1: E265 block comment should start with '# '
Line 30:1: E302 expected 2 blank lines, found 1
Line 32:9: E265 block comment should start with '# '
Line 37:9: E265 block comment should start with '# '
Line 39:26: E128 continuation line under-indented for visual indent
Line 43:9: E266 too many leading '#' for block comment
Line 50:33: E231 missing whitespace after ','
Line 54:33: E231 missing whitespace after ','
Line 61:33: E231 missing whitespace after ','
Line 76:9: E265 block comment should start with '# '
Line 77:27: E231 missing whitespace after ','
Line 77:43: E231 missing whitespace after ','
Line 78:57: E231 missing whitespace after ':'
Line 107:9: E265 block comment should start with '# '
Line 114:1: E265 block comment should start with '# '
Line 116:1: E265 block comment should start with '# '
Line 118:9: E265 block comment should start with '# '
Line 120:30: E231 missing whitespace after ','
Line 121:9: E265 block comment should start with '# '
Line 124:9: E265 block comment should start with '# '
Line 126:30: E231 missing whitespace after ','
Line 129:32: E231 missing whitespace after ','
Line 132:32: E231 missing whitespace after ','
Line 135:32: E231 missing whitespace after ','
Line 138:9: E265 block comment should start with '# '
Line 140:31: E231 missing whitespace after ','
Line 143:30: E231 missing whitespace after ','
Line 145:49: E231 missing whitespace after ','
Line 146:29: E231 missing whitespace after ','
Line 149:9: E265 block comment should start with '# '
Line 151:33: E231 missing whitespace after ','
Line 154:9: E265 block comment should start with '# '
Line 155:50: E203 whitespace before ','
Line 156:30: E231 missing whitespace after ','
Line 158:9: E265 block comment should start with '# '
Line 160:9: E265 block comment should start with '# '
Line 162:9: E265 block comment should start with '# '

Comment last updated on July 18, 2018 at 11:52 Hours UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Jul 15, 2018

Thanks for the pull request @wtbarnes! Everything looks great!

@Cadair Cadair added this to the 1.0 milestone Jul 15, 2018

wtbarnes added some commits Jul 15, 2018

@Cadair Cadair added the [Review] label Jul 18, 2018

@Cadair

Cadair approved these changes Jul 18, 2018

@nabobalis
Copy link
Contributor

left a comment

Can we add dask to the CI files so the test can run?

@wtbarnes

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2018

I believe dask-core is already a dependency of scikit-image so I don't think Dask even needs to be explicitly installed. I guess if this is true, we also don't need to try-except the dask import

@Cadair

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

@wtbarnes we should try except it as I dont think array is part of the core pacakge, not sure though. Still would rather not rely on transitive dependencies.

@nabobalis nabobalis merged commit c4c69fd into sunpy:master Jul 18, 2018

6 of 7 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Giles Click details to preview the documentation build
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
sunpy-bot All checks passed

@wtbarnes wtbarnes referenced this pull request Aug 15, 2018

Open

Dask FITS loader #2715

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.