-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
First draft of Spectrum1D class #1
Conversation
This is going to be quite ugly-formatted, but I'm going to copy over the comments that went through via e-mail previous to this pull request for posterity: @wkerzendorf test = Spectrum1d.from_dispflux(linspace(4000, 5000, 1000), ones(1000)) There's still quite a few simple bugs in there and it is not completely fleshed out. What are your thoughts for the general structure? @astrofrog
Oops, ignore my comment about relative imports, since this is an @crawfordsm Just as there is a short cut for dispersion and flux, should their be a short cut for disp_units and flux_units? I was thinking that the doc string for the class should be expanded to include the information about what it does inherent from NDData (particularly units and meta--since these are functions which are availalbe, but not immediately obvious unless you are familiar with NDData). Although I'm not sure if this is unnecessary duplication (or if there is a better way to include this), but should the following be included (mostly copied directly from NDData):
so there's now: Spectrum1D.from_array(disp, flux) the same as before just with different names Spectrum1D.from_table(table_name, disp_col='disp', flux_col='flux')
crawfordsm: I agree that the docstrings are not very informative yet. But the plan is definitley to get that into a better state.
My 2 cents: I don't like using anything with a scipy dependency. If
For example, if linear interpolation is requested (default), you can use numpy, and otherwise you could import scipy on the fly. So to the user, the switch is transparent, but if they don't have scipy installed they get an Exception. |
Adrian and I met this afternoon and discussed the code. Below are the thoughts we had. Before going into them, I'm going to reiterate that I'm coming from a place where I feel we should write to your average (i.e. mostly programming-novice) physicist - not to programming experts. They outnumber us, but we want them to feel as comfortable programming as possible. Also, please take all of this in the spirit of constructive criticism. :)
Comments and feedback certainly welcome. :) Cheers, |
Re: smoothing The proposed approach is:
I believe to add a function as a class method, you need to do something ugly like:
Is there any nicer way to implement this? If not, I'd argue against that approach; something more like atpy's reader registration feature would be preferable. |
@keflavich Re: adding a function to an existing class, here's a toy example:
|
1 - I agree with this, it does indeed not make sense to initialize from anything other than two 1-D sequences (numpy array, list, tuple), because it is easy enough to specify two columns from a table, from a file, etc. and Spectrum1D should not really be implementing I/O. I think this does not mean that in future, one could not imagine having a 'read' method for well defined 1-D spectrum specific formats that don't have any ambiguities though. 2 - Also agree, I keep stumbling every time I see 'disp' too! 3 - Astropy has the requirement that if scipy is not available, it should not break the imports, so if scipy is not available, the interpolate method would have to be disable. Therefore, I am suggesting that for linear interpolation, np.interp should be used so that at least basic interpolation is available if scipy is not installed. But I'm all for allowing more complex interpolation if scipy is installed. 5 - I agree that operations are too ambiguous. I think there should be functions such as:
which allows the final range to be specified for example. 6 - I agree with having two methods instead of one with a text option, but I'm opposed to referring to the indices as 'pixels'. I would use 7 - This is basically down to the debate between in-place modifications and returning the modified value. I can see arguments both ways... 9 - +1 for the 10 - I wonder whether for smoothing we couldn't have:
etc which would be easier than having people assign their own function to the class. |
Regarding copying - if we did go with methods that modify the spectrum in place, we could define a
Instead, they could just do:
|
#3. OK, I did not know that. I think that's the right approach. I'd suggest throwing a custom SciPyDependency exception that contains more detail/explanation for the user as appropriate. #6. I like #7. I considered my typical case: I have a spectrum, I change the redshift, I interpolate, etc. If I'm given a new spectrum at each step, that's a lot of object creation/memory usage/property copying. I think returning a new array as might be typical in IDL is the right choice, but these are objects and I'd want to minimize object creation, particularly if I'm going to analyze thousands of spectra (I am). #10. As with point 6 above, I prefer not passing in a free text value as a parameter. Nothing is being saved in the implementation: it will be one long if/elif/elif statement. The user can only pass in values that we've implemented anyway, so to me
is cleaner than:
The custom option could be
Or something. But I'm open to suggestion. Providing a custom... oh, that's the answer. There's a SmoothSpectrum object, we define an API, and they implement it. They pass the object to the Spectrum1D class and it uses it to do the work. OO, baby! Re copying: The reason I suggested the |
@demitri - github's markup edited your EDIT: github auto-marks things like |
Now my own list (and thanks @adrn for the well-organized discussion points!): 1 - I mostly agree with @astrofrog (and @adrn and @demitri) - we don't want any of the io stuff actually implemented here... The Put another way, all the class functions @wkerzendorf proposes could be moved to specutils.io.from_* and I think then everyone's happy? 2 - Agreed, except that "dispersion" is a bit long to type for something we may be using a lot (in an interactive/ipython setting). What about having 3 - +1 for @astrofrog's suggestion 4 - I'm not sure what @adrn is saying here - is this a general comment or something about this particular implementation? Are you saying you think we should not allow 5 - I'm not as sure about this one - I think arithmetic operations should be always allowed on scalars to alter the flux - that is , Also, I think it should be safe to do 6 - I agree with using the method name instead of a parameter setting, and +1 to @astrofrog's suggestion 7 - I like the idea of an However, I'm not sold at all on modifying internal state. Why not instead have those functions make a copy themselves, and return the copy? Then you needn't ever worry about accidentally overwriting your raw data... I guess in this case (unlike @astrofrog), I don't see the virtue of in-place modification (although I could be sold on it). If there's a lot of disagreement here, the best thing is probably to have an 8 - I definitely like what @adrn proposes here. Unfortunately, it steps on the work that needs to be done in the 9 - +1 here - 10 - Whatever we do here should absolutely be consistent with what is doen for 11 - I have no strong opinions here - those that do should set the rules :) Whew! |
@astrofrog @adrn re: copying: @demitri @keflavich @adrn re:smoothing and adding methods: I think it's dangerous to encourage the
But novice users following documentation are more likely to do
And then get confused which one they're using. One solution, as @keflavich suggested, is to attach smoothing methods to instances, but I agree that's very clunky. So I like @demitri's final suggestion of having a SpectrumSmoother (better name than SmoothSpectrum because it's clear that's an action rather than just a smooth spectrum) API, with the addition that I would suggest including a registry - then you can either do:
or
|
One final comment based on @wkerzendorf's PR here: we definitely should not use the |
@adrn I know we've kind of got a free-form discussion going, but maybe you should consider creating a second pull request based on adrn/specutils@bc8dbd8 ? Although maybe we want to avoid splitting the discussion at this point - your call. |
@demitri - regarding the
is more clunky than
In fact, we can push for most Astropy objects to have this method for consistency. I think at this stage, the fundamental debate is whether Spectrum objects should be modified in place, or copied and returned. Python lists follow the former:
does not return a copy. To return a copy, the user has to do:
The value of in-place modification is that you save memory, and if you are doing e.g.
after each operation, you would be creating a new plot rather than updating the existing one. If we did have in-place modifications, we should provide all methods as functions which do return copies, as for At this stage, I would be against opening another pull request, as it will scatter the discussion, but if there is a fundamental divide in the group regarding in-place modifications and returning copies, then ultimately I say we implement both and submit it to the list to see what people think (but I think it's much better to have a tangible example that people can try out, than just having a theoretical discussion). |
Regarding a new pull request, I also think we should avoid splitting discussion at least for now. When we come to some more solid conclusions about the suggestions, I'll create an updated list and that can be appended to a new pull request. Here are some thoughts from me: 1 - @eteq regarding your point about from_table() -- I think the idea here is that Great comments everyone -- loving the discussion! |
Regarding convolution, I don't understand the need for a SmoothSpectrum class. Why not just allow the user to specify a smoothing function? (would be accessible to more people that way) |
Topic: inplace vs. copy - I'd like to see some methods that do both explicitly. For example, I'd like to see X and Y unit conversion like:
On other things, like interpolation and smoothing, I'm ambivalent and therefore would like to see both options implemented e.g. with an "inplace" flag. I'm entirely in favor of the Topic: smoothing methods - I like the idea of registering smoothing functions, but agree with Tom that there's no need for a class (just a clear specification for what the function accepts and returns). |
First of all, thanks for providing a working example, that really helps when trying out different implementations. Sorry for the long delay in answering - I was travelling. I hope everything that goes on here on the list is taking in the spirit of constructive criticism ;-). Well here's my suggestions for the different points. Instantiator 1I agree 100% there should only be one way to instantiate the Spectrum1D object. In my opinion this is init(self, data, error=None, mask=None, wcs=None, meta=None, What I merely did was include the read-in methods, that you guys mentioned, within the Spectrum1D object. Obviously we could put them somewhere in specutils.io, but why? IMHO, they do belong to Spectrum1D as they are only useful with a Spectrum1D object. It would obviously be reflected in the doc_string of Spectrum1D, how to use these read methods. A spectrum dispersion solution is in the core a world coordinate system. If you reduce spectra with IRAF or other software for that matter, one often fits some polynomial to get a mapping between pixel and dispersion space. I think it's not a good idea to just force every user to map out his WCS into an array, as there's definitely a loss of information that is important in some cases. As for my implementation it just set WCS equal to the lookup table (dispersion array), for now. But this is only a temporary measure until there's a proper WCS in place that deals with lookup tables. With a proper WCS class, when .disp is required it would return a lookup table generated from the true WCS. #verbosity 2 I agree in most cases to be verbose. I think however that disp for now is fine. We spoke in great length about what we wanted to call that axis (and after 'wave' was shot down and that rightly so ;-) ), we decided between dispersion and disp. disp won and I still think that's a good name. It would be at a prominent place in the Spectrum1D docstring. We could have both I guess. #Scipy 3 I share your opinion on that one, but it seems most of the other developers don't (as there seems to be some problem installing it on older systems). It was decided to try to use only numpy wherever possible. And I think we can by with that most of the time. #Slicing 4 #Operations 5 @eteq has made a good point that at least adding constants and adding spectra on the same wavelength grid should be allowed. I have also had a lot of success with interpolating spectra on a common wavelength grid. pysynphot (I think) also does and many people like that. The meta dictionary could store the settings how it arithmetic behaves (e.g., arithm_mode = 'inner range' or equivalent. If the current implementation of addition does not suite a user, they can always make other arithmetic functions or subclass it. However, I still believe that an "inner range"-implementation will suite most people for simple arithmetic. Options as text 6I don't quite get what the problem with text options is: Your suggestion is to have the text stored in the function name rather than as a parse-in option for a standard function. For many things this will duplicate code (e.g. smooth boxcar and smooth gaussian will probably both rely on the same convolution library just passing different parameters to it). Secondly, if there's different text options users can look the up easily in the docstring of the common function (e.g. spectrum1d.convolve). If there's different function names then the user will have to go to the documentation to figure what is available. If one is worried about too many if statements, we can just use a dictionary. I might misunderstand, but if we can't use text options we have to use integer IDs (e.g. for convolutions boundary treatment: 1=wrap, 2=mirror, 3=const, etc.). This would go against item 2 in your list asking for verbosity. Finally, many scientific python library like numpy, scipy and matplotlib make extensive use of text options interpolation 7I believe that most operations on the spectrum should return a new Spectrum1D object, as does the interpolation function in both numpy and scipy. I think most reduction and analysis software will create a new object on an operation like interpolation or convolution for that matter. As for the memory. If the old spectrum is not needed any more, one can just overwrite it (e.g. spectrum = spectrum.interpolate(arrange(6000,7000))). Internally a new array is created anyways if we use numpy or scipy functions (and then overwrites the old one). Error arrays 8I do very much agree that this is a very important point, but believe that this is more of an NDData issue than Spectrum1D. I believe we should save this issue for later and now discuss the other issues at hand. Plotting 9In item 1 there was talk that we should not include io in the spectrum object as it is not essential to the spectra (I wonder if essential is the right word). I believe that plotting faces exactly the same arguments. As was discussed in the Google+ session - everyone wants to plot a spectrum, but everyone also wants to load a spectrum from a fits file, text file, etc … . We should think about where to draw the line on this. Maybe we should have a specutil.plot module which has lots of option (like line id etc) Smoothing 10Similar to interpolation I believe it should return a copy. I think this is an issue, we should think after figuring out initialization routines and so on (however important). SpectrumCollection 11Good point! There I again believe that the WCS (and using the NDData init function) will help us a lot as it can store only one dispersion solution and make it available for all spectra in the collection. The other main point you bring up here is: Spectrum1DBase. That might be a good idea to have a very simple base class from which we inherit other Spectrum classes (maybe OpticalSpectrum, Xray spectrum). It's definitely worth thinking about it. So to proceed, I think it one of the main problems is how to instantiate the function (including IO). I created a discussion thread here () to separate this issue. Thanks again for the well structured list and bringing up the suggestions! |
Sorry forgot the link to the other issue: #2 |
It sounds to me like now that we've all had a chance to ponder these issues, a telecon would be a more efficient way to go through each point and make a decision in each case? Regarding I/O, this is not really important because it would only involve adding some methods, so the decision regarding that can wait until later anyway. More important is the general behavior of the class (in place mods vs returning copies - and I don't believe we should have a flag to choose that, we should just pick one and go with it). |
I agree a telecon would be more productive. Before we have that though I'd like to propose we start making a list of features we'd like Spectrum1D to implement, so we can discuss that at the same time. I think the progression should be "desired functionality" -> "API" -> "implementation", and we seem to be working the other way around (although lots of great discussion came from it!). I'll start a new pull request for that. Argh. Not a pull request. I just want to create a new thread. I don't get this GitHub thing. |
@demitri - so you mean basically we fist should decide what we want a typical user script to look like, and then worry about making it work? We could even do it the fun way and do test-driven development (where we write the tests using what the code should look like, then implement it to make all the tests pass!). |
Actually, I'm skipping a step again, I agree we should worry about functionality before even API... |
@astrofrog It sounds reasonable, yeah. :) The are several reasons for this approach. At the very least, when someone has time to write code there will be clear pieces to implement, essentially a to do list. As you can guess, I value a clean API very highly. If it's not clear or easy to use, it doesn't matter how good the code underneath is. People (justifiably) won't use it. Implementation can always be changed underneath. |
I guess everyone here is interested in a clean API. I also agree that is should be as easy as possible to use, but this is supposed to be a class that caters to most astronomy fields and such I worry about oversimplification. I don't believe with most things AstroPy that one will just be able to pick it up without having a glance of the documentation once. That is definitley not the case for all other reduction and data analysis tools (be it IDL, matlab, mathematica, numpy, scipy, ....). Anyways +1 on a telecon (maybe Thursday or Friday?). |
@wkerzendorf - remember that Spectrum1D doesn't have to do everything for all wavelengths, we can always sub-class Spectrum1D for more specific applications (OpticalSpectrum1D, XRaySpectrum1D, etc.). So in that sense, Spectrum1D should be fairly simple. |
@wkerzendorf Let me try to explain my position. I strongly believe that "power" is not at odds with "clean and easy to use", but to get both requires a great deal of thought and consideration (which I am happy to do). It means keeping things as consistent and self-evident as possible. Let me give an example. There is a scientific code written in C++ called ROOT that is to the particle physics community what IDL is to astronomy. Everyone uses it, new students are immediately started on it. It is one of the most poorly designed frameworks I've ever seen (this is saying a lot). Consequently, people who use it are always cursing it, wasting incredible amounts of time to do straightforward things, and take a long time to "get up the learning curve". As an example, there is a Histogram object that you create from an array. (As you know, particle physicists can't get up in the morning without making a histogram.) The simplest thing you would do is create the histogram object, populate it with the array, and then plot it. If you did that, it's possible that you're plot wouldn't made sense, because the first bin of the histogram contains underflow values, and the last bin overflow values. This is not expected behavior. The histogram is an object - the underflow/overflow should be two properties. Virtually every ROOT user has run up against this, and in my (last) department alone I've seen hours wasted on figuring this out. In my mind I multiplied that by hundreds (more more) all of the other ROOT users, and that's a ridiculous amount of physicist time wasted by a poor design decision. There are many, many more examples like that. Yes, people will have to go to the documentation. That is unavoidable. But the code should be as readable as possible, such that if you are simply reading someone's code - and you are not intimately familiar with AstroPy or Spectrum1D - that the intent is clear. If you are writing it, sure, you'll have to look at a tutorial or the docs to get the details right. I recently converted someone to use IDL from Python, and one thing he was so excited about is that there was something he wanted to try but didn't know if it would work. He said he wrote some lines that, based on his using Python, that he thought should work, and was thrilled that they did. This is a huge reason for constancy with the greater Python in general.* Another reason why I think the code should be easy to use is pragmatic: if it's not miles better in ease of use, then people who are entrenched in IDL will simply stay there. Simple as that. We don't have a hope to compete with the existing IDL libraries in the short term, so ease of use becomes even more important. Finally, one more lesson I learned from ROOT. I remember looking at the documentation which was written by the developers (this was years ago). I quickly discovered the docs were very sparse and had holes in many, many places (and this in the reference describing the API!). The main docs started off with a statement that said (I'm paraphrasing), "The docs are written by the developers. It's not as complete as it can be, but we feel our time is better spent in developing the code further than writing documentation." What they were saying is that felt their time outweighed the countless hours that thousands of physicists have wasted trying to figure out how to get a job done, and I thought that was unbelievably arrogant. (I am not leveling that accusation at anyone here!!) But it's a lesson that's stuck with me; if I need to spend 5 hours to get a piece of code right that will save someone five minutes I will happily do it, because that five minutes has a huge multiplier next to it. I'm willing to make someone type the extra six characters in "dispersion" over "disp" for this reason. (I'm also willing to introduce people to editors that feature code-completion. :) Anyway, I'll get off my soapbox. Simple + powerful = win. |
I was thinking about it, and I'm going to +1 @astrofrog's mention of Python's
Someone who knows Python will see the convention, and someone who doesn't will learn when they see it applied elsewhere. The details to handle that (and use the same code) are easily, um, handleable. The main reason I'm opposed to returning copies is that if I am working with hundreds or thousands of spectra (SDSS recently crossed the 1 million spectrum milestone, so I will), generating new copies at each step (regridding, reshifting, etc.) will generate a huge number of (OO) objects as everything internal to the class is copied. This is a high drag to any OO code - you want to avoid creating large numbers of objects. Further, my spectra are going to come from a database, so I'd like to place the returned database object (which contains a huge volume of metadata and a pointer to a live connection to the database). Copying that into a new object (or any other user metadata that might come along for the ride), again, could be a big drag on the code at best or break things at worst. |
Hi everyone, I've been interested in astropy (and more specifically in Spectrum1D since it's formal conception). For those of us who did not attend the meeting, I'm glad that the discussion has been brought more "online" so that others can participate; it seemed there was a lot discussed at the meeting which was not immediately reflected in the wiki. For the background I currently use a custom class (like what Spectrum1D will eventually be) for my data analysis with tens of thousands of spectra where in the final stages of analysis they are all on a common dispersion. 1 - I think there are actually two issues present here; (i) whether to have the from_ascii/from_fits methods at all, (ii) whether they should be in the Spectrum1D class or in specutils.io and (iii) how to handle instantiating a Spectrum1D object if you want to allow for (a) arrays of dispersion and flux and (b) flux and a WCS dispersion mapping. i - The from_ascii/from_fits (and to_ascii/to_fits) are common enough that they should be used, and they should live in specutils.io. ii - The general way to instantiate a Spectrum1D object should be with a flux and dispersion array (with the units specified by disp_units and flux_units). I understand that if you instantiate a spectra from a FITS image which contains some polynomial WCS map for the dispersion, then information is lost if you simply feed in all the dispersion points as an array into Spectrum1D. Ideally you might want to have a WCS mapping for every object so that maximum information is retained, but the general case will be when people instantiate a class with two arrays. Moreover if you want a WCS mapping for every object, then when the dispersion is linear you are only adding a layer of unnecessary complexity and overhead. If an object is instantiated from a FITS file or from an ASCII file then it would be prudent to keep the WCS dispersion. Thus, the from_ascii/from_fits functions are no longer simply "wrappers" for np.loadtxt, they actually provide additional functionality because your resultant Spectrum1D will have a better described dispersion - without the need to specify the dispersion units either. I do not believe this is sufficiently worthy to have two types of Spectrum1D classes, or two different instantiating methods. I'm open for discussion on the implementation of this, but allowing a astropy.wcs class (or whatever the appropriate class is) to be passed through as the dispersion is probably the best way to go. The dispersion can then be specified as either an array or a astropy.wcs class, and the only overhead in the instantiator for most cases will be
or similar. Some may believe that this could give reason for multiple different use-cases requiring different if-statements in the instatiator causing unnecessary complication, but this is a logical fallacy. The general case is for when a dispersion and flux array will be passed in, and the only other way you would want to instantiate it would be if you wanted to provide additional dispersion information through the use of a WCS mapping. 2 - Verbosity is great, but it should not be forced for those who know what they're doing. Have the documentation all state the argument as dispersion, but just as matplotlib allows me to specify "edgecolor" or "ec", let me specify disp instead of dispersion. FYI I have never read "disp" as display in these discussions. Just like you read RCE as rice, I read it as race. Let the user who knows what they're doing use a shortcut - just don't advertise the shortcut. 3 - +1 for @astrofrog 4 - Spectrum1D is a custom class, and its usage should be designed for the most general of cases. If I'm wanting to take certain parts of a spectrum, I would rarely be splicing it based on flux levels. In general I would want to cut out certain portions of the spectrum. Using,
is natural and clean. Am I referencing the indices or the dispersion? The idea of having this class and having it interact with other spectra and scalars is that I shouldn't have to worry about the indices in a general sense. I work with the dispersion and that's the level I want to interact on. What about in the scenario above there? What does 450 actually represent physically? It ought to slice on whatever the natural units of the dispersion are. If the dispersion units are nm, then I should get a resultant spectra from 450nm to 580nm (or a shorter region if bounded by the length of I know that some people feel uncomfortable with this because it's typically how they handle lists or arrays. But the difference here is that the natural way to associate with lists and arrays is to splice by their indices because you have no other mapping available. Here I don't want to have to work on the index space, I always work in the dispersion space which is familiar to me. Having a Write it for the way people use their existing spectra; we access arrays by indices, we work with spectra on dispersion. If you've got a dispersion layer there you ought to encourage the use of it! 5 - Here's how I think it should go: Scalar operations are a no-brainer, they should operate on the flux. If two spectra are on the same dispersion mapping then it's also quite simple; the fluxes should be operated on simply. The metadatas should be updated together in the resulting spectra. If there is missing information (e.g. RA header in one object but not in the other) then the resulting spectrum should keep as much metadata as available. If there is conflicting headers then the first operand metadata should be used but a warning should be expressed. Similarly if two spectra are not on the same disperson mapping then it should use the first operands dispersion mapping and throw a warning. In my code I would never write code like this (for the same reasons I would not use a known deprecated function in code), because it would spit out unnecessary errors. But for those of us just wanting to try things out in the terminal and "get things done", it should be just as easy to do so - but I should be warned of any potential complications. Addition of the errors is a seperate issue - if the Spectrum1D is associated with gaussian errors, and another spectrum is associated with similar errors then the two errors should know how they should operate when added together. It should not be Spectrum1D's job to assume how the error classes should interact with one another. 6 - Don't clutter up the namespace, especially when smooth_x and smooth_y may access the same underlying functions with different arguments. Scipy allows text arguments, numpy does, matplotlib does; let's not clutter the namespace. 7 - I'm a firm believer that Spectrum1D (and nddata objects) should return new class objects. If I'm manipulating my spectrum (either by subtracting sky, interpolation, whatever the operand is), I want to keep data concurrency. I want to know how the spectrum was affected so that I can analyse the steps later on. The stages where I don't need to keep the original data before the operation, I would simply re-write over my object:
But since in general it's good to have data concurrency and know what happened to the spectra at each step, I don't want to be forcing a deepcopy every time I want to do an operation. Return new objects. The ones that aren't needed would be overwritten by the user anyways. 8 - Not a relevant issue yet. 9 - "Smart" plot commands are not necessary! Why bring in matplotlib overhead when it's not required? I understand that in other libraries people have spent way too long trying to plot something. The documentation for astropy will not be lacking, and there should be examples on how to plot heaps of different data types. Nobody is ever going to use spectrum.plot() as the final plot they use in a journal paper because there is heaps of detailed customisation (axes ticks, titles, labels, colours) that would need to be adjusted before publishing it. In my opinion there should be easily accessible examples on the web of how to easily plot different types of data, but we should not be wrapping matplotlib! Not to save the 4 lines it takes to do:
The data sets for Spectrum1D are simply not sufficiently complex to warrant a "smart" plotting function which could easily be shown as a documented example code. 10 - Once again, return a copy. If a copy is not required the user would have just overwritten their existing spectrum anyways. Only lists are sorted in place, numpy allows for sorting in place and for returning a new object. 11 - An interesting problem and I can greatly see the need for it. I think this issue should be split to a separate discussion. |
By our telecon on Friday, I will compile a list of discussion points for each of Demitri and my ideas -- I would have done it sooner but have been swamped with work lately! |
So @adrn does this mean you prefer Friday noon (is that true for Demitri as well)? I think for now we should stick to the two main issues of instantiator and copy vs inplace for this telecon. This will probably take a long time and has implications for many of the other points. |
@wkerzendorf Friday noon works for me, not sure about @demitri -- agreed about the latter point, but I think it'll be good to collect our input before the discussion. Thanks! |
@wkerzendorf - I can do Thursday or Friday noon (assuming you mean EST). I do not think we should limit the discussion to just copy vs inplace and instatiator, however... Looking over this list, I think less than half of the 11 originally mentioned are now in contention (or, alternatively, can be naturally merged). So @wkerzendorf, can you post here a short list of the items that we have not yet achieved consensus regarding? |
And I would also suggest that, following the telecon, we close this pull request after someone posts a summary of what decisions were made, and we can use new issues for anything that's still not resolved (or new issues we want to poll online). |
A few comments/summaries I hope might clear a few items up (I'm intentionally leaving out quite a bit that is probably better resolved on the telecon): 2 - @andycasey So if I'm reading you correctly, having 3 - @wkerzendorf Actually, the standard we've come to in the Astropy core is that we can't require scipy at import - but having functionality that requires it is just fine. It may mean some functionality is lost if you don't have scipy installed, but in this case it should be pretty easy to default to the scipy version, and fallback on the numpy one if an ImportError is raised. In general, though, it's ok to use scipy if there's no easy alternative, as long as it's only imported in the function/method 4 - @andycasey, I think I'm more and more in line with @demitri and @adrn on this one: In this class we should not assume much about what the user wants... and allowing slice notation will always require assumptions that in many cases will be violated. The original intent (at least as I understand it) of 5 - I think there's a general consensus that scalar operations and matched dispersion operations are ok, but non-matching dispersion is not as definite. (My feeling is the same as 4 - don't assume anythin in 9 - @andycasey and @wkerzendorf I think this would in some cases be used for publication-quality plots... As long as there's some flexibility in the provided options, I can see using this for "real" plots. I've certainly done this with I should emphasize also that this would not mean adding matplotlib as a requirement. As I described above in 3, the intent in astropy is to encourage use of external libraries when needed as long as it doesn't crash at import. So a user without matplotlib can use 11 - Also sounds like there's consensus that this should at least be supported... I think it fits well with the suggestion I have above of layers of "smartness" in a class heirarchy. |
I didn't comment on slicing previously because I had to think about it - my opinion is that that square bracket slicing should not be implemented because it would not be intuitive whether this would be index/wavelength/frequency based. Furthermore, if we do have slicing, it should be through |
Thursday or Friday should be fine for me. |
Both Thursday or Friday are ok for me. |
Sorry, my collaborators just suggested putting an HST proposal in, so I'm out for Thursday or Friday. Early next week? (I can do weekend as well if people are really keen). |
Monday or Tuesday (Feb 27/28) @ noon EST work for me. I could also do Sunday if others want to (although I would prefer weekday). Or perhaps a doodle is called for here (ideally with the "could make it work but prefer not" option)? Also, @wkerzendorf, do you still intend to post a list of what needs to be decided and what needs to be? I think we will be more productive if we have all the items on the table and agree we need to make a decision on all of them in the telecon (or at least agree to post the telecon consensus and give a bit of time to make sure the rest of the community doesn't strongly disagree). |
@eteq sounds great. I do intend to post a list, but haven't put it online yet. My idea, for the next meeting is to restrict it to the two main issues (instantiator; inplace/copy) so it doesn't get too long (and schedule subsequent issues for later meetings). I will however have a list with all current issues. Will do a doodle as well. |
@wkerzendorf - My vote would be try to get through most of the issues in one meeting if possible - that's why I was suggesting we at least have all the topics. I think if we have a clearly-defined agenda we can get through the items in a relatively short amount of time. But we can decide that as a group once the call starts. Good luck with the Hubble proposal! |
@wkerzendorf - Are we not going to do this next week after all? A doodle should go up at least a few days in advance... |
We are I'll put the doodle up in an hour. |
redacted and replaced with new draft |
Improved documentation and some consistency impovements
change 'generic' to 'tabular' (there is no "generic" FITS spectrum)
APE1 tweaks
APE1, the meta-APE
Getting most up-to-date version of cubeviz
Fix and unify internal links.
* WIP: Compat with asdf v3 * Fix schema references and move ASDF schemas a few level up. Co-authored-by: William Jamieson <wjamieson@stsci.edu> * TST: Stop using roundtrip_object * Fixes and cleanups (#1) * Fix the schema * Fix tags to be the ones listed in the manifest * Remove uncecessary tree packing/upacking. asdf converters no longer need to return a completely valid ASDF sub-tree. ASDF will recursively parse the tree until everything is converted automatically. * Add `spectral_axis-1.0.0` tag for `SpectralAxis` objects * Add `SpectralAxisConverter` * Fix broken tests * Rename converters to reflect new ASDF language * Better organize asdf extension * Rename `spectra` module to `converters` to better describe its contents * tmp fix jsonschema bug * Move jsonschema pin to test section because the pin will happen upstream anyway and should not be in our install pins. Other nitpicks. * Simplify create_spectrum1d in testing * Undo jsonschema pinning, update asdf pin Co-authored-by: William Jamieson <wjamieson@stsci.edu> --------- Co-authored-by: William Jamieson <wjamieson@stsci.edu>
* WIP: Compat with asdf v3 * Fix schema references and move ASDF schemas a few level up. Co-authored-by: William Jamieson <wjamieson@stsci.edu> * TST: Stop using roundtrip_object * Fixes and cleanups (astropy#1) * Fix the schema * Fix tags to be the ones listed in the manifest * Remove uncecessary tree packing/upacking. asdf converters no longer need to return a completely valid ASDF sub-tree. ASDF will recursively parse the tree until everything is converted automatically. * Add `spectral_axis-1.0.0` tag for `SpectralAxis` objects * Add `SpectralAxisConverter` * Fix broken tests * Rename converters to reflect new ASDF language * Better organize asdf extension * Rename `spectra` module to `converters` to better describe its contents * tmp fix jsonschema bug * Move jsonschema pin to test section because the pin will happen upstream anyway and should not be in our install pins. Other nitpicks. * Simplify create_spectrum1d in testing * Undo jsonschema pinning, update asdf pin Co-authored-by: William Jamieson <wjamieson@stsci.edu> --------- Co-authored-by: William Jamieson <wjamieson@stsci.edu>
looking forward to your comments.