astropy
Quick plot method for Spectrum1d
This PR is an output of discussions from the April spectroscopic sprint week - there was a consensus there that there should be some light-weight quick-look plot that yields a static matplotlib plot.
There was a fair amount of associated discussion which I can add to and ping folks in if there's further discussion in this PR (there's notes somewhere from the discussion which I'll try to find and link later).
TODO's remaining:
- [ ] Make it clear that this is intentionally limited to simple static visualiztion. Any more complex UI stuff belongs in other packages rather than
specutils - [ ] Include uncertainty as a shaded region if present
- [ ] Have an option to switch to "line-like" plots instead of the step/histogram-style plots. (There was broad agreement the step-like approach is the preferred default)
- [ ] Make the tests actually meaningful (currently it just makes sure the function runs, instead of checking that the plot makes sense)
I personally have strong feelings against including plotting functionality (especially as methods directly on the Spectrum1D class), as this really feels like a separation of concerns issue, and I don't want to set a precedent for having to support particular external packages.
In this case, it might be simple enough, maybe, that it's alright to include, but I can imagine getting flooded with tickets about why this might not work with certain data, or requests to extend or change for specific corner cases. I'm just not convinced that there's a generic enough use case that satisfies everything without playing favorites among spectral representations that couldn't better be solved by documentation discussing how to parse Spectrum1D objects for use in any plotting library.
I feel a bit the same way about the visualization stuff that is in Astropy core or photutils. It doesn't sit all that comfortably in those packages. As a user, I tend to go roll my own before I realize that it's even there, then kick myself for not having gone looking for it.
I wonder if it would make more sense to have a separate Astropy-affiliated visualization library that would offer ways of visualizing 1,2 and 3D data sets that make use of the common Astropy infrastructure for units, time, coordinates, etc.
Revisiting this topic from the distant past: the motivation behind this was from the spectroscopy workshop in spring 2020, where almost all of the scientists present said "I really want a quick-and-dirty plot method, we've all rolled our own already and want to not have to maintain it".
I voiced exactly the same concerns as @nmearl here, and everyone there was basically like "but it's so important you have to include something. We're fine with it having a big warning label that says 'we will not add lots of features, if you want a fancier plot do it yourself'!". I see it as meeting the "80% of quick-look needs" threshold, which at least the folks at the workshop agreed what's in this PR seems to meet, and I agree with @nmearl and @hcferguson that going further than that is impossible-to-maintain scope creep.
So I'm willing to pick this up again if @nmearl and or @hcferguson are convinced (or others want to speak up in defense - I know @kelle, @eblur, and @aburgasser all voiced interest in this). One thing I definitely take from the above though: we need to be very clear that the scope of this method is limited and we are not going to customize it for a bunch of cases. I think a warning is going too far, but do we think a note in the docstring is sufficient to make the point, or is there something stronger we can do?
It's worth noting, @eteq, that NDCube has built-in plotting functionality. Since we're moving toward subclassing that for Spectrum1D instead of NDDataRef, maybe take a look at their plot API and functionality to see if it will make more sense/is possible to build off of that?
In fact, @Cadair currently has a PR open to rework their quickplot API, and they were looking for someone to pick up the baton there and finalize that PR if you want to check it out.
I suggest that this should not be merged until it's refactored to use a bin_edges that the user can easily recalculate, and some view on the flux data that allows easy plotting, so that we can include documentation that shows something like:
pl.plot(sp.bin_edges, sp.bin_edge_matched_data, drawstyle='steps-mid')
I'll add that spectral-cube should be a user of specutils and definitely wants this functionality:
https://github.com/radio-astro-tools/spectral-cube/issues/716
I started chiming in yesterday without having read the whole thread, so those comments are relevant to this PR, but there is also a higher level discussion here that we might want to punt to another forum.
Somewhere in the astropy ecosystem, we need easy to access (from the user's perspective) visualization tools that are also very close to the data. One of the primary use cases for this is just to check if something worked. For example, you normalize the continuum of your spectrum, or subtract it: you then need to look at that spectrum and make sure it is indeed normalized/baselined. I don't think this use case is satisfied by big, external visualization packages
I understand both sides of the argument presented here: as an end user, my primary need is to look at the data, so there needs to be some visualization tool. As a developer, visualization development sucks and can easily sabotage package development.
The customer is right here, though. So, if we don't put the quick, close-to-data, matplotlib-based visualization tools here (in specutils), then where?
By inheriting from NDCube[1] (once sunpy/ndcube#401 is merged and 2.0 is out) you will be getting a .plot() method which will default to a WCSAxes line plot (in pixel space) as long as matplotlib is installed (otherwise .plot() will error). There was some talk about adding step plot functionality (sunpy/ndcube#398) which was postponed until post sunpy/ndcube#401.
Happy to make sure that ndcube provides sane defaults or at least knobs to turn for specutils.
[1] As opposed to NDCubeBase.
In some discussion at the astropy coordination meeting I think we decided two things:
- This PR is still useful as an implementation of what a group of real-live scientists said they wanted in a spectroscopic quick-look tool
- With ndcbue 2.0 now in specutils, I should re-jigger this PR to use its fancy plotting machinery (e.g. sunpy/ndcube#401).
Something to add (came out from the discussion in the Hack Session at Spectro workshop (11/2023)): Include the astropy.visualization.quantity_support inside the function directly. This is needed to support unit-based selections in matplotlib.
