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

Size of signal.py #943

Closed
dnjohnstone opened this Issue Apr 6, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@dnjohnstone
Contributor

dnjohnstone commented Apr 6, 2016

signal.py is getting very big and is expanding rapidly as people add more and more functionality.

What is, or should be, our strategy for keeping this manageable? Presumably some separation of code to different places in a rational way, but what might the different sections of code be?

@francisco-dlp francisco-dlp added this to the Discussion milestone Apr 6, 2016

@francisco-dlp

This comment has been minimized.

Member

francisco-dlp commented Apr 6, 2016

+1000!

Actually this touches a deeper issue that we should discuss. Currently we have Signal1DTools, Signal2DTools, Signal (which inherits from the previous two), Image and Spectrum. Image and Spectrum inherit from Signal, and just add some extra few methods. This means that these objects are highly cluttered. The rationale was that, because we can configure the "navigate" attribute of the axes at will, having all these methods always available meant that we could e.g. while working with a spectrum image set the axes in "image configuration", apply an image method, and go back to "spectrum configuration", without actually transforming the class (which currently requires rearranging the data in memory).

I would like to propose the following alternative:

  • Get rid of Image and Spectrum in favour of Signal1D and Signal2D (I think that this was proposed by @dnjohnstone a while ago). Signal1D = SpectrumSignal1DToolsSignal and Signal2D = ImageSignal2DToolsSignal
  • Add a copy_data argument to as_image and as_spectrum (to be renamed as_signal1D, as_signal2D).

What do you think?

@dnjohnstone

This comment has been minimized.

Contributor

dnjohnstone commented Apr 7, 2016

Ok I'm all for your suggestion @francisco-dlp - I can't remember where we had that discussion before, but that is also my recollection of the conclusion.

I'm happy to do the grunt work on initially separating out the code, the question is when? ASAP and for v1.0.0 or (given it will break so much for everyone) - do we take this as a 1.1.0 target and warn everyone now that it's coming?

@francisco-dlp

This comment has been minimized.

Member

francisco-dlp commented Apr 8, 2016

I think that 1.0 is appropriate for this. Even 0.8.5 (due very soon as we've fixed some few important issues) would be as long as we don't modify the current API but simply add the new Signal1D and Signal2D classes and add deprecation warnings to Spectrum and Image. In this way, we'll be able to fully get rid of them for 1.0.0.

@francisco-dlp francisco-dlp modified the milestones: next_maintenance_release, Discussion Apr 8, 2016

@francisco-dlp

This comment has been minimized.

Member

francisco-dlp commented Apr 8, 2016

I've set the milestone to next_maintenance_realease. @dnjohnstone, are you happy to undertake the split? The changes to as_image and as_spectrum can be introduced in a different PR to master instead.

@dnjohnstone

This comment has been minimized.

Contributor

dnjohnstone commented May 26, 2016

Addressed by #963

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment