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

Added dtype argument to combiner #178

Merged
merged 3 commits into from
Oct 31, 2014
Merged

Added dtype argument to combiner #178

merged 3 commits into from
Oct 31, 2014

Conversation

heidtna
Copy link
Contributor

@heidtna heidtna commented Oct 27, 2014

Related to #176.

Added the argument to reduce memory usage by letting the user specify the dtype, and setting the default at 64 if none is specified.

@mwcraig
Copy link
Member

mwcraig commented Oct 29, 2014

@crawfordsm -- do you mind if I merge this? The only travis fail was related to an out-of-date astropy_helpers, which I've updated separately in #179.

@crawfordsm
Copy link
Member

Thanks @heidtna for adding this in! Sorry to take a while to look at this, but can I ask you to add two more things:

  1. Can you update the doc string Combiner class to include the dtype parameter?
  2. Can you add a test for this? ie what happens if an invalid dtype is passed?

It might also be good

Although I'm happy if this is done in two steps--merging this pull request and then a second pull request to add the testing and documentation, but I think it would be better to add it in here if easy to do.

@mwcraig
Copy link
Member

mwcraig commented Oct 31, 2014

Thanks @heidtna, merging this in!

mwcraig added a commit that referenced this pull request Oct 31, 2014
Added dtype argument to combiner
@mwcraig mwcraig merged commit 854b88e into astropy:master Oct 31, 2014
mwcraig added a commit to mwcraig/ccdproc that referenced this pull request Nov 5, 2014
Added dtype argument to combiner
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