-
Notifications
You must be signed in to change notification settings - Fork 5
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
U/jrbogart/rotate galaxies #96
Conversation
…ata column as list rather than np.array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions, otherwise looks fine.
''' | ||
Parameters | ||
----------- | ||
cols list of column names belonging to the file | ||
mask if not None, use it and return compressed array | ||
no_np if true, do not return as np.array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please give an example in this docstring describing when one want to use no_np = True
.
dat['hp'] = hps[i] | ||
# special handling for for tophat SEDs | ||
for k in ['sed_val_bulge', 'sed_val_disk', 'sed_val_knots']: | ||
dat[k] = c.get_native_attribute(k, no_np=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have just unpacked the numpy arrays here instead of making a more complicated interface for .get_native_attribute
:
dat[k] = [_ for _ in c.get_native_attribute(k)]
and would have put a comment here regarding the need to do this so that the nature of the special handling is explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added explanatory comments here and in parquet_reader. The extra argument to get_native_attribute is a complication, but it seemed better to not do something at all rather than do it and then undo it.
skycatalogs/scripts/rotate.py
Outdated
|
||
# Now write out healpixels for this collection (sets of healpixels | ||
# belonging to different fields are disjoint) | ||
# for hp in hps_distinct: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this commented out code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the last line. I have my doubts whether the sentence in the first two lines is even true. It would depend on how close the fields are to each other.
Add a script to rotate galaxy data from an existing catalog to a new location and output new catalog files.
Also includes a backwards-compatible update to parquet reader, adding an optional argument to specify data should be returned as a list rather than np.array