Analytic functions with blackbody#3094
Conversation
There was a problem hiding this comment.
Better to use a context manager [1], with np.errstate(...), so a badly timed exception/ctrl-c doesn't leave the numpy error state differently.
[1] http://docs.scipy.org/doc/numpy/reference/generated/numpy.errstate.html
There was a problem hiding this comment.
I'm inclined to think this shouldn't be an option at all, since np.seterr contexts can be set at any level in code execution and should probably be managed outside any specific functions (do we have any other funcks that do this?)
There was a problem hiding this comment.
if warnings are common in this function, then I'd be ok with mentioning it in the docstring but I agree it shouldn't be an option.
There was a problem hiding this comment.
The only other code I see that does this is in the new-ish all_world2pix implementation:
Line 1521 in 7aedf59
But I would make the same point there--it should probably be taken out or at least changed to use np.errstate.
There was a problem hiding this comment.
Okay, I'll take it out and just put an example in the doc.
|
@pllim - can you change rebase this to include @skendrew commits too (from #1480)? We want her to get credit for writing the initial stuff. You can just cherry pick her three commits, and then insert one in between that justs deletes them again, but then it preserves the version history and ensures she gets included in the contributor lists and such. |
CHANGES.rst
Outdated
There was a problem hiding this comment.
(minor): can you change this to something slightly more explicit, like, "The astropy.analytic_functions was added to contain analytic functions useful for astronomy"?
|
@pllim - I know you mentioned it, but there's not any of her actual commits in the log, so she doesn't get credit from the various scripts and such we use to track contributions based on commit authors. |
|
And if you're not sure what I mean about grabbing the relevant commits, I'm happy to do that in my repo and then you can reset this to match mine. |
|
@eteq , I am indeed not sure how to do that properly. Your help would be appreciated, thanks! |
There was a problem hiding this comment.
Best to also test the function with a single wavelength.
There was a problem hiding this comment.
Actually, now that I look more closely, this particular test requires integration under the curve, so single wavelength cannot be tested here, but it is tested elsewhere.
|
@pllim - see https://github.com/eteq/astropy/commits/rebased-planck - if you just do |
|
@pllim - the tests seem to be failing for numpy < 1.9... You may need to special-case the numpy-scalar situation... (perhaps only also if numpy is < 1.9?) |
|
@eteq , based on reviews above, I removed that line in the code. So in the next commit, that particular failure won't be an issue anymore. FYI. |
|
@astrofrog , image file removed, tests passed. |
|
@pllim - if you wouldn't mind, could you squash the last five commits so that we avoid including the binary image in the history of the repo? (otherwise it will still count for the repo size). Thanks! |
There was a problem hiding this comment.
in_x not being a Quantity does not guarantee that it does not have a unit already. E.g., with this a column with Angstrom units will become a Quantity with units of A**2. Instead, I would test on the object not having a unit and then letting the Quantity initializer take care, using again the equivalency framework (this makes things slightly slower for, say, a Column with frequency units, but I think that's OK since this is not the common path):
if not isinstance(in_x, u.Quantity):
with u.add_enabled_equivalencies(u.spectral()):
in_x = u.Quantity(in_x, u.AA)
There was a problem hiding this comment.
I don't think it's necessary to do the full conversion here, because it is done anyway in blackbody_nu() later. But I can change isinstance(in_x, u.Quantity) to hasattr(in_x, 'unit').
There was a problem hiding this comment.
Yes, that test should work too (assuming it passes u.spectral_energy(in_x)), but still rewrite the test so that Column with unit unset goes through and do use u.Quantity(in_x, u.AA), as it is substantially faster:
if getattr(in_x, 'unit', None) is None:
in_x = u.Quantity(in_x, u.AA)
In [6]: a = np.arange(100.)
In [7]: %timeit a * u.AA
10000 loops, best of 3: 42 µs per loop
In [8]: %timeit u.Quantity(a, u.AA)
100000 loops, best of 3: 7.34 µs per loop
There was a problem hiding this comment.
What exactly do you mean by "Column with unit unset"? I am not familiar with Column.
There was a problem hiding this comment.
In [15]: c = Column([1.,2.,3.], name='a')
In [16]: c.unit
In [17]: c.unit is None
Out[17]: True
So, if I had a table with a column with its unit unset, the code should assume the unit is Angstrom. But if it is, say, nm, it should pass the thing through.
There was a problem hiding this comment.
I did a quick test, passing in Column gives wrong answer because it is not being converted to Hz properly. I think Column support for unit conversion is out of scope for this PR.
There was a problem hiding this comment.
But yes, I can implement getattr and u.Quantity(in_x, ...) anyway. Just be clear, that does not mean Column gives the correct answer although nothing crashes.
There was a problem hiding this comment.
I'd do the getattr anyway, and indeed not worry about Column otherwise. There has been endless discussion about how its unit should be treated, and I don't want to hold this up for that!
|
@pllim - two final small comments (sorry for them being a bit late, just got back from a short holiday). With those, good to go. |
…nd documentation.
…d by Marten v. K.
|
@mhvk , done and done. @astrofrog , I think this is ready to merge, because I don't think the failed tests are related to my PR. |
|
@pllim - thanks! I'm just testing it out against some of my code and will merge if it agrees :) |
|
Ok, looks good to me and agrees with my previous codes, so merging! If anyone decides they want to discuss the package name more, we can still do it before 1.0 (but I like analytic_functions) |
Analytic functions with blackbody
See #3077