Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix from table #24

Merged
merged 7 commits into from Apr 9, 2013

Conversation

Projects
None yet
3 participants
Member

wkerzendorf commented Apr 7, 2013

Fixed up from_table-classmethod to add documentation and masks and flags. This should be merged after #23.

Member

wkerzendorf commented Apr 8, 2013

@keflavich @hamogu any comments? if not will merge in 6 hours

@hamogu hamogu commented on the diff Apr 8, 2013

specutils/spectrum1d.py
flux = table[flux_column]
- disp = table[dispersion_column]
+ dispersion = table[dispersion_column]
if uncertainty_column is not None:
uncertainty = table[uncertainty_column]
@hamogu

hamogu Apr 8, 2013

Member

Should we check that flux.units and uncertainty.units are the same? Spectrum1D expects them to be the same (which is a sensible design decision, I think).
I can easility imagine cases, where the input table has e.g. fluxes in 10^-7 erg/s/cm^2 and uncertainities in 10^-9 erg/s/cm. This is not a place to convert the units, but we could issue a warning if they don't match.

@wkerzendorf

wkerzendorf Apr 8, 2013

Member

@hamogu done! Check it out and if you're happy we can merge.

@keflavich keflavich and 1 other commented on an outdated diff Apr 8, 2013

specutils/spectrum1d.py
if uncertainty_column is not None:
uncertainty = table[uncertainty_column]
+ if uncertainty.unit is not flux.unit:
@keflavich

keflavich Apr 8, 2013

Contributor

Is this the right check? i.e., are uncertainty.unit and flux.unit the same object? Usually I'd think == is more appropriate, but not sure.

@hamogu

hamogu Apr 9, 2013

Member

+1 to not ... == ... , or more to !=

@keflavich keflavich and 1 other commented on an outdated diff Apr 9, 2013

specutils/spectrum1d.py
else:
uncertainty = None
- return cls.from_array(flux=flux.data, disp=disp.data, uncertainty=uncertainty, dispersion_unit=disp.units, unit=flux.units)
+ if isinstance(flag_columns, basestring):
+ flags = table[flag_columns]
+ elif misc.isiterable(flag_columns):
+ flags = FlagCollection(shape=flux.shape)
+ for flag_column in flag_columns:
+ flags[flag_column] = table[flag_column]
+ else:
+ raise ValueError('flag_columns should either be a string or a list of strings')
@keflavich

keflavich Apr 9, 2013

Contributor

a list -> an iterable

@hamogu

hamogu Apr 9, 2013

Member

@keflavich While you are certainly right, I kind of like the simpler language of a list. In 90% of the use cases it probably is a list of strings and for a python beginner the term "an iterable" is a hard nut to crack.

@keflavich

keflavich Apr 9, 2013

Contributor

@hamogu I generally agree with you. Maybe for precision's sake, we could say a list (or an iterable)? It's a little more verbose, but I think it would be good to know as a user if a tuple or array is acceptable (saves me from doing arr.tolist() thinking that only lists are OK)

@keflavich keflavich commented on an outdated diff Apr 9, 2013

specutils/spectrum1d.py
else:
uncertainty = None
- return cls.from_array(flux=flux.data, disp=disp.data, uncertainty=uncertainty, dispersion_unit=disp.units, unit=flux.units)
+ if isinstance(flag_columns, basestring):
+ flags = table[flag_columns]
+ elif misc.isiterable(flag_columns):
+ flags = FlagCollection(shape=flux.shape)
+ for flag_column in flag_columns:
+ flags[flag_column] = table[flag_column]
+ else:
+ raise ValueError('flag_columns should either be a string or a list of strings')
+
+ return cls.from_array(flux=flux.data, dispersion=dispersion.data, uncertainty=uncertainty,
+ dispersion_unit=dispersion.units, unit=flux.units, mask=table.mask, flags=flags)
@keflavich

keflavich Apr 9, 2013

Contributor

Line wrap? I can't see the end of this line on github, so I assume its >80 chars. (others below too)

Contributor

keflavich commented Apr 9, 2013

Only a few minor changes needed, then it can be merged.

Member

wkerzendorf commented Apr 9, 2013

@keflavich @hamogu anything else - otherwise will merge in 6 hours.

wkerzendorf added a commit that referenced this pull request Apr 9, 2013

@wkerzendorf wkerzendorf merged commit d00f357 into astropy:master Apr 9, 2013

keflavich pushed a commit to keflavich/specutils that referenced this pull request Sep 21, 2017

Merge pull request #24 from bsipocz/patch-1
Adding cookiecutter APE to the list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment