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.

Sign up for GitHub

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

Jump to bottom

datastack module and optional requirements #22

Closed
olaurino opened this issue May 20, 2015 · 9 comments · Fixed by #190
Closed

datastack module and optional requirements #22

olaurino opened this issue May 20, 2015 · 9 comments · Fixed by #190
Assignees
@olaurino
Labels
area:code note:can-workaround priority:medium type:bug
Milestone
4.8.1

Comments

@olaurino
Copy link
Member

As mentioned in the 4.7 release notes, the new datastack module fails to load when matplotlib is not installed.

This is a side effect of the decision to have the IO and plotting backends as soft dependencies, while in CIAO there is always a back-end installed.

Moreover, if matplotlib is installed, but pyfits is not, the datastack module will indeed be loaded, but the tests will fail because pyfits is not available.

The datastack should check for the presence of the backends and act accordingly.

Installing matplotlib and pyfits works around the issue.
@olaurino olaurino mentioned this issue May 20, 2015
Merged
@DougBurke
Copy link
Contributor

On a possibly-related note, in tests I've been doing to set up some sphinx documentation, I've had problems with the autosummary extension (in particular, the astropy_helpers.sphinx.ext.astropyautosummary version) because the sherpa.image.DS9 module raises an error when imported and DS9 is not available. I haven't had time to track down what's going here, but thought I'd raise it here (as it's related to how Sherpa deals with optional dependencies, even if it turns out to be a different problem).

@DougBurke
Copy link
Contributor

@olaurino - from your original comment, do you know what "act accordingly" means, or does this need to be worked out? I assume that if a backend is missing then either:

a) there's a warning message saying something like "datastack support is not available" and the module is not loaded

b) the module will load, but will do nothing when a backend is missing.

I guess option a fits in better with the current approach of Sherpa (but I haven't looked at the tests/code to see which might make more sense functionally).

@olaurino
Copy link
Member Author

@DougBurke

option a) is what happens now. The module fails to load and the user cannot use it.

Some of the module's features do not have anything to do with plotting, and they should be available even if plotting backends are not installed.

So, I believe we should make the module work like in option b), with some of the functionality always available, and some other functionality available only if a plotting backend is installed.

Also, we should refactor the parts of the code that directly probe for the installed backend. First, we should define common interfaces that can be factored out so that the main code gets the implementation from a factory. Then we should evaluate whether we want to fold this new abstraction layer of the datastack module into the main sherpa backend implementations themselves, but at that point this should just be a matter of moving things around.

We should also make sure we track the issue you were having with DS9 and sphinx, and in general this could be part of the larger review of the backends design.

To summarize, here is what I believe we should do:

  1. factor the plotting-backend-specific code into a specific datastack submodule. If no plotting backends are available at runtime, the code that does not have to do with plotting should still work, while the plotting calls should provide a helpful warning.
  2. refactor the plotting specific submodule to extend the backends' interface, so that new implementations can be added in an Object Oriented fashion.
  3. if appropriate, move this extension to the backend implementations in the main sherpa code.

DougBurke added a commit that referenced this issue Jul 25, 2015
@DougBurke
Split datastack plotting code into sub-modules.
08d3d4d 
This commit

  - "abstracts" the plotting code
  - creates versions of this abstraction of matplotlib, chips, and no backend

The primary driver for this change is given in the comment to #22:
#22 (comment)

Notes on this change

  - the test suite does not seem to exercise the plotting code as it
    did not find several issues during development (requiring manual testing);
    this commit does not change any test code

  - the original version included a set of matplotlib-specific code; this
    has been made available to ChIPS users too (e.g. plot_savefig/plot_xlim)
    but some discussion is probably warranted on if this is wanted, and
    if it is what the interface should be

  - (related to previous point) the _print_window function seems there
    for ChIPS users (there's a savefig for matplotlib users); I am
    not happy with the name
@DougBurke
Copy link
Contributor

@olaurino is @08d3d4d what you were thinking of?

@olaurino
Copy link
Member Author

Indeed. Eventually it would be good if the backend was returned by some kind of factory, but that's something we need to improve in the main sherpa code as well.

As for testing, I hope I can commit an example unit test. The unit tests should exercise the expected behavior of the datastack code by mocking the backends and just ensuring that the right backend calls are properly made. The integration of the actual backend is more complex, and depends on the backend. Since the addition of the datastack code is recent and it is a reasonably isolated module, I would try and use it as an example of how we should do proper testing and use this example in other parts of the codebase.

@olaurino olaurino added this to the 4.8 milestone Jul 27, 2015
@DougBurke
Copy link
Contributor

I don't plan on turning the feature/datastack-backend-support branch - currently at @08d3d4d - into a PR in the near future, as I think it really needs tests before being added (and PR #71 includes clean up of the datastack test suite, so do not want to change things until it is closed).

@olaurino olaurino assigned olaurino and unassigned DougBurke Aug 20, 2015
DougBurke added a commit that referenced this issue Dec 23, 2015
@DougBurke @olaurino
Split datastack plotting code into sub-modules.
a4099a2 
This commit

  - "abstracts" the plotting code
  - creates versions of this abstraction of matplotlib, chips, and no backend

The primary driver for this change is given in the comment to #22:
#22 (comment)

Notes on this change

  - the test suite does not seem to exercise the plotting code as it
    did not find several issues during development (requiring manual testing);
    this commit does not change any test code

  - the original version included a set of matplotlib-specific code; this
    has been made available to ChIPS users too (e.g. plot_savefig/plot_xlim)
    but some discussion is probably warranted on if this is wanted, and
    if it is what the interface should be

  - (related to previous point) the _print_window function seems there
    for ChIPS users (there's a savefig for matplotlib users); I am
    not happy with the name
@DougBurke
Copy link
Contributor

@olaurino what needs to be done to fix or close this ?

@olaurino
Copy link
Member Author

olaurino commented Jan 8, 2016

I have a branch ready to be turned into a PR:
https://github.com/sherpa/sherpa/tree/feature/datastack-backend-support

What's holding me is that it relies on OTS packages that are not in CIAO, so a PR would break CIAOD (unless we label it pr:hold, which we can do). I am about to send requests for the inclusion of this packages (mock and pytest).

Also I am using this as a prototype to explore more robust ways of isolating unit tests, to employ modular, reusable fixtures, to parametrize tests, and so on. So I am trying to document these patterns both inside and outside the code.

I put it on the back burner this week to start reviewing and merging open PRs, but I'll get back to it soon.

@DougBurke
Copy link
Contributor

ta. I thought that with the recent build/integration PRs that have gone in, this looked a good candidate for inclusion.

@olaurino olaurino mentioned this issue Feb 1, 2016
Closed
olaurino added a commit that referenced this issue Mar 4, 2016
@olaurino
fix #22 - datastack can be used if no plotter available
24230eb
olaurino added a commit that referenced this issue Mar 4, 2016
@olaurino
fix #22 - datastack can be used if no plotter available
dfdb930
olaurino added a commit that referenced this issue Mar 4, 2016
@olaurino
fix #22 - datastack can be used if no plotter available
518baac
@olaurino olaurino mentioned this issue Mar 4, 2016
Merged
olaurino added a commit that referenced this issue Mar 4, 2016
@olaurino
fix #22 - datastack can be used if no plotter available
c363e76
olaurino added a commit that referenced this issue Mar 17, 2016
@olaurino
fix #22 - datastack can be used if no plotter available
7eeba31
olaurino added a commit that referenced this issue Mar 25, 2016
@olaurino
fix #22 - datastack can be used if no plotter available
2366c8a
@DougBurke DougBurke mentioned this issue Mar 25, 2016
Closed
olaurino added a commit that referenced this issue Mar 30, 2016
@olaurino
Merge #190 (olaurino) - fix #22 - datastack can be used if no plotter…
260af96 
… available
@olaurino olaurino self-assigned this Mar 30, 2016
@olaurino olaurino mentioned this issue Jun 29, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@olaurino olaurino

Labels
area:code note:can-workaround priority:medium type:bug
Projects
None yet
Milestone
4.8.1
Development

Successfully merging a pull request may close this issue.

fix #22 - datastack can be used if no plotter available sherpa/sherpa
2 participants
@DougBurke @olaurino