Make Column or MaskedColumn name arg be a keyword arg name=None #731
Conversation
|
This looks good to me. Do you want to add the docs and tests here, or in a separate pull request? |
|
I'll add docs and tests here. |
|
👍 on improving the consistency with Numpy -- this tripped me up the first time I saw it, too. |
|
👍 |
|
As far as I'm concerned, this is done. Tests pass locally. You can look at the new red warning here: @iguananaut - this should be backported to 0.2 if possible since it provides advance notice of pending API change in 0.3. |
|
This looks good to me! Maybe someone else should just double check too, but then it's good to merge. |
|
Looks fine to me, as well. |
This is implemented as a decorate on __new__(cls, name=None, data=None, ..)
for Column and MaskedColumn.
The warning is:
In the next major release of astropy (0.3), the order of function
arguments for creating a {class_name} will change. Currently the order is
{class_name}(name, data, ...), but in 0.3 and later it will be
{class_name}(data, name, ...). This is consistent with Table and NumPy.
In order to use the same code for Astropy 0.2 and 0.3, column objects
should be created using named keyword arguments for data and name, e.g.:
{class_name}(name='a', data=[1, 2]).
This is only complete for table and io.ascii. All of these tests run without generating warnings. This was done to be sure that all code in astropy.table and astropy.io.ascii is fully compliant with the new Column args checking, i.e. it will never happen that a call to astropy.Table can generate a warning to the user.
|
@astrofrog @eteq - based on discussion in #726, I made a decorator around I ran all tests for This is sort of a bigger change than I was hoping for, but I think it's good to actually generate warnings instead of just docs. This way there should be absolutely no issue with the arg order change in 0.3. |
| def wrapper(*args, **kwargs): | ||
| if len(args) > 1: | ||
| cls = args[0] # Column or MaskedColumn class from __new__(cls, ..) | ||
| warnings.warn(WARN_COLUMN_ARGS_MESSAGE.format(class_name=cls.__name__)) |
There was a problem hiding this comment.
This is not a major thing, but might be nice: perhaps you should use a specific Warning subclass here? Either a custom one you define just for this, or maybe just DeprecationWarning (this isn't exactly a deprecation, but it's similar). That makes it easier to put in the warnings filter if someone doesn't want the constant barrage of messages. (Although maybe that's what you want?)
There was a problem hiding this comment.
The first version did exactly this and used FutureWarning. But this gave a slightly weird output so I went back to the default.
WARNING: FutureWarning: In the next major release of astropy (0.3), the order of function
arguments for creating a Column will change. Currently the order is
Column(name, data, ...), but in 0.3 and later it will be
Column(data, name, ...). This is consistent with Table and NumPy.
...
There is no barrage, after the first warning you never see it again in that session (unlike Quantity float conversion for instance). I was imagining the user would set WARN_COLUMN_ARGS to False to disable this entirely. I purposely did not document this fact because nobody should ever do that, they just need to fix their code...
There was a problem hiding this comment.
Ah, I see - I was thinking you meant to have this warn every time, rather than just once, and I didn't notice WARN_COLUMN_ARGS.
I don't see where WARN_COLUMN_ARGS is used, though. shouldn't there be an if WARN_COLUMN_ARGS(): above the warning call?
There was a problem hiding this comment.
I don't see where WARN_COLUMN_ARGS is used, though. shouldn't there be an if WARN_COLUMN_ARGS(): above the warning call?
Doohh! Good catch. Fixed in f91436c.
There was a problem hiding this comment.
Great, looks good now, then.
I wonder if we should be using ConfigurationItems when we know the setting will be temporary? (that is, presumably it will disappear in 0.3). No need to change this here since it's all ready - I'm just sort of thinking out loud. And anyway, this'll be a good excuse to implement the configuration file updating scheme we said we wanted to have by 0.3 :)
|
Other than that one suggestion, this looks good to me. |
|
This looks good to me! |
Make Column or MaskedColumn name arg be a keyword arg name=None
Make Column or MaskedColumn name arg be a keyword arg name=None
Make Column or MaskedColumn name arg be a keyword arg name=None
The first arg for the
ColumnorMaskedColumnisname. In this PRnameis changed to a keyword argumentname=None. This makesnameoptional, but more importantly would allow the documentation to recommend creating a column with:This would get people writing code with
dataandnameboth provided as kwargs, which would smooth the transition to 0.3 --> we would also include in the documentation a heads-up that the plan is to change the keyword argument ordering in 0.3 toColumn(data=None, name=None, ...), so that from 0.3 you could write:As mentioned in #726 this ordering is consistent with
Tableandrecarray. This PR is in lieu of actually changing the order now. That would be possible but is a significant API change (which in theory is still open for discussion @iguananaut) affecting many files and would likely delay release of 0.2If this PR instead is accepted for 0.2 then the documentation update and additional tests will be provided.