Skip to content
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

Pretty image title #610

Merged
merged 8 commits into from
Sep 21, 2018
Merged

Pretty image title #610

merged 8 commits into from
Sep 21, 2018

Conversation

wtgee
Copy link
Member

@wtgee wtgee commented Sep 19, 2018

Some fixes to the making of the image title. Follow up from comment in #493

  • Consistency with title parameter.
  • No default title if nothing passed (making a title is very slow on a raspberry pi).
  • Sneaking a few cleanup items in.

@jamessynge this does not add any coordinates to the title. The info dictionary has the coordinates for the field but I'm not sure that makes the most sense as pointing could be off. Coordinates for the center don't exist yet because this comes before plate-solve.

* No default title if nothing passed (making a title is very slow on a raspberry pi)
* Sneaking a few cleanup items in
@wtgee wtgee requested a review from jamessynge September 19, 2018 12:22
@wtgee
Copy link
Member Author

wtgee commented Sep 19, 2018

Exposure time would be nice on there. The fits_to_pretty does this already. Should actually move that default title out of the fits specific method and build it in process_image.

field_name = info['field_name']

image_title = '{} {} {}'.format(field_name,
seq_id.replace('_', ' '),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this implies to me that we should have two ways of tracking sequence id: the string needed for file paths and another for presentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Observation object itself should have a __str__ method that is better, but that wouldn't help much here as this is what is being passed around for the eventual FITS headers. But yes, fundamentally I agree. We pull this replace a lot.

@@ -63,32 +63,31 @@ def crop_data(data, box_width=200, center=None, verbose=False):
return center


def make_pretty_image(fname, timeout=15, **kwargs): # pragma: no cover
""" Make a pretty image
def make_pretty_image(fname, title=None, timeout=15, **kwargs): # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding an issue to add testing of this? It doesn't strike me as that hard to test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#608 also contains an item about adding testing for images. We currently don't test more of our images.


This will create a jpg file from either a CR2 (Canon) or FITS file.

Notes:
See `$POCS/scripts/cr2_to_jpg.sh` for CR2 process
See `/scripts/cr2_to_jpg.sh` for CR2 process.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove $POCS? Assuming well founded, why not also remove the leading slash?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why I did this. Will fix.

filter_type = header.get('FILTER', 'Unknown filter')
date_time = header.get('DATE-OBS', current_time(pretty=True)).replace('T', ' ', 1)
if not title:
field = header.get('FIELD', 'Unknown')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps 'Unknown field' and 'Unknown duration'?

field = header.get('FIELD', 'Unknown')
exp_time = header.get('EXPTIME', 'Unknown')
filter_type = header.get('FILTER', 'Unknown filter')
date_time = header.get('DATE-OBS', current_time(pretty=True)).replace('T', ' ', 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could read the file time as a source of a date.


title = '{} ({}s {}) {}'.format(title, exp_time, filter_type, date_time)
norm = ImageNormalize(interval=PercentileInterval(percent_value), stretch=LogStretch())
norm = ImageNormalize(interval=PercentileInterval(clip_percent), stretch=LogStretch())

fig = plt.figure(figsize=figsize, dpi=dpi)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reminder about my request that we have images (plots) that can display on both white and black backgrounds.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would normally happen with the fig.savefig method, which is part of the OO interface. I've added a note in #608.


if kwargs.get('primary', False):
cmd.append('link')
if title:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No default title?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, see PR description.

@wtgee wtgee merged commit ae51586 into panoptes:develop Sep 21, 2018
@wtgee wtgee deleted the pretty-image-caption branch September 21, 2018 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants