add make_pretty_from_fits file to develop #319

jermainegug · 2018-01-12T00:41:30Z

*creates norm variable
*checks wcs (**issue)
*changed cmap from cubealpha_r to inferno

try: wcs = WCS(fname) except: wcs = False
This does not work as expected.
If we do:

wcs = WCS(fname) print(wcs)

We get this as a result:

Number of WCS axes: 2
CTYPE : ''  ''  
CRVAL : 0.0  0.0  
CRPIX : 0.0  0.0  
PC1_1 PC1_2  : 1.0  0.0  
PC2_1 PC2_2  : 0.0  1.0  
CDELT : 1.0  1.0  
NAXIS : 3352  2532

*creates norm variable *checks wcs (issue) *changed cmap from cubealpha_r to inferno

wtgee · 2018-01-12T00:46:31Z

Thanks @jermainegug. So when I told you to develop it I had you work with a separate script because you didn't have POCS installed yet. But know that you do...

What we want to do is take your _make_pretty_from_fits and repalce what is in: https://github.com/panoptes/POCS/blob/develop/pocs/utils/images/__init__.py#L87. Then you can just go ahead and delete this file.

wtgee · 2018-01-12T00:46:58Z

You can just keep working in the same branch and any new changes you push will automatically become part of this PR.

*using the function in make_pretty_from_fits file *delete make_pretty_from_fits file

AnthonyHorton · 2018-01-12T01:07:02Z

Ah, OK, going to need to try something else to check for a valid WCS. Seems WCS(fname) will just create a mostly empty template in the absence of a full WCS in the FITS headers. Check the astropy.wcs documentation, there must be a function for checking whether a WCS is present/valid or not.

Also, for the plot using WCS use RA & dec for the axes (not an overlay) and don't bother with Galactic coordinates, at least not by default. Could make the choice of coordinate systems an option, of course.

wtgee · 2018-01-12T01:09:04Z

Just about to put the comment in review for WCS. We do this elsewhere. Basically:

w = wcs.WCS(filename)
assert w.is_celestial

wtgee · 2018-01-12T01:09:50Z

pocs/utils/images/__init__.py

+    wcs = False
+
+    """
+    try:


try: w = wcs.WCS(filename) assert w.is_celestial except AssertionError: pass

Or just skip the try and use if wcs.is_celstial below.

Get this error message AttributeError: 'bool' object has no attribute 'WCS' for w = wcs.WCS(filename).

@jermainegug That's because in your code you're setting wcs to False, whereas @wtgee's code suggestion is assuming that wcs is astropy.wcs (i.e. you're doing from astropy import wcs)

Yes, I didn't adjust it for your code, I just copied what we have elsewhere.

You import WCS directly so use that instead of wcs.WCS.

But, you are also mixing types. You create wcs = False (a boolean) up above and then assign wcs=WCS(filename) (a WCS class) to it. I would just skip the try and use the is_celestial in the if statement directly after getting the wcs.

What is wcs in this case? wcs=WCS(fname)?

R.T.D.

It's a WCS object, i.e. an instance of the astropy.wcs.WCS class.

http://docs.astropy.org/en/stable/api/astropy.wcs.WCS.html#astropy.wcs.WCS

In retrospect, probably best to use a more distinct variable name for it, e.g. image_wcs.

wtgee · 2018-01-12T01:18:29Z

pocs/utils/images/__init__.py

@@ -6,7 +6,10 @@
 from matplotlib import pyplot as plt
 from warnings import warn

+from astropy.io import fits


I don't think you are using this.

Nope, using that in my jupyter notebook.

*fixed check for wcs *not sure about RA & dec axes

codecov · 2018-01-12T02:00:07Z

Codecov Report

Merging #319 into develop will decrease coverage by 0.75%.
The diff coverage is 83.87%.

@@             Coverage Diff             @@
##           develop     #319      +/-   ##
===========================================
- Coverage    83.81%   83.06%   -0.76%     
===========================================
  Files           51       51              
  Lines         3367     3454      +87     
  Branches       422      439      +17     
===========================================
+ Hits          2822     2869      +47     
- Misses         404      440      +36     
- Partials       141      145       +4

Impacted Files	Coverage Δ
pocs/utils/images/__init__.py	`82.75% <83.87%> (-2.68%)`	⬇️
pocs/state/states/default/tracking.py	`83.33% <0%> (-16.67%)`	⬇️
pocs/utils/rs232.py	`79.64% <0%> (-13.98%)`	⬇️
pocs/scheduler/observation.py	`93.44% <0%> (-6.56%)`	⬇️
pocs/dome/astrohaven.py	`72.38% <0%> (-6.29%)`	⬇️
pocs/utils/database.py	`89.13% <0%> (-4.35%)`	⬇️
pocs/observatory.py	`84.84% <0%> (-1.36%)`	⬇️
pocs/state/states/default/pointing.py	`87.75% <0%> (ø)`	⬆️
pocs/scheduler/field.py	`100% <0%> (ø)`	⬆️
pocs/mount/simulator.py	`92.79% <0%> (+0.26%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f90359a...3c7ebf9. Read the comment docs.

*just set axis labels to RA and dec

AnthonyHorton · 2018-01-12T02:53:16Z

@jermainegug Have a look through the documentation on this under the astropy.visualization module: http://docs.astropy.org/en/stable/visualization/wcsaxes/index.html

For this you should be using matplotlibs object orientated interface, rather than the pyplot interface, see the second example in the above link.

That will then make it easier to properly format the tick labels for axis, see: http://docs.astropy.org/en/stable/visualization/wcsaxes/ticks_labels_grid.html You're going to want to do this in order to make the Right Ascension axis use hour units instead of the default degrees.

Also, you need to make the axis labels more explicit. I'd prefer to spell out Right Ascension & declination (if there's room) but more importantly you should specify the coordinate system and (if applicable) equinox that are being used. The WCS object should have this information (and also allow you to transform to others). It's probably either FK5 J2000 or ICRS.

EDIT: There's an example of setting tick label format via the object orientated interface in the Huntsman dither.py utils: https://github.com/AstroHuntsman/huntsman-pocs/blob/develop/huntsman/utils/dither.py

*descriptive axes *coordinate system in title? *how to get equinox?

wtgee · 2018-01-12T04:08:41Z

pocs/utils/images/__init__.py

@@ -89,7 +90,7 @@ def make_pretty_image(fname, timeout=15, **kwargs):  # pragma: no cover
 def _make_pretty_from_fits(fname, **kwargs):
    config = load_config()

-    title = '{} {}'.format(kwargs.get('title', ''), current_time().isot)
+    title = '{} {} {}'.format(kwargs.get('title', ''), current_time().isot, 'WCS') # is WCS the coordinate system?


You can do current_time(pretty=True) for a better time output here.

And WCS = "world coordinate system", but you don't just want the string there unless you are only putting it there when is_celestial is true or something.

Even then it will be obvious from the rest of the plot whether a WCS from the FITS image has been used or not, from the axis labels, tick labels and grid. Don't need to put it in the title too.

So we will just be able to tell the coordinate system based off what the image looks like? Thus no need to have anything writing out that it uses WCS.

wtgee · 2018-01-12T04:15:02Z

pocs/utils/images/__init__.py

-        ax.set_xlabel('RA')
-        ax.set_ylabel('Dec')
+
+        ra = ax.coords[0]


This is not really an ra, it is an ra_axis. Same with the dec. Better to be explicit about the name because if you had another variable (which maybe you will in future) that actually holds an RA value you would get confused by this.

wtgee · 2018-01-12T04:17:09Z

pocs/utils/images/__init__.py

+        ra = ax.coords[0]
+        dec = ax.coords[1]
+
+        ra.set_axislabel('Right Ascension')


I tend to do label [units] (although I think @AnthonyHorton does something different), in which case this would be Right Ascension [hms] and Declination [dms].

Yes, I tend to prefer the label / units convention (i.e. the numbers are the quantity in question divided by the given unit). Either is fine, though.

In this particular case putting units in the axis labels should be redundant though, the tick label formats include units, see https://github.com/AstroHuntsman/HuntsmanPOCSLegacy/blob/develop/examples/notebooks/dice9.png for an example.

I have written them in for now. Will make sure that the units are in the axis label when I run the code with a fits file that has wcs., then I will get rid of the units in the label.

wtgee · 2018-01-12T04:17:33Z

pocs/utils/images/__init__.py

-        ax.set_xlabel('Pixel x-axis')
-        ax.set_ylabel('Pixel y-axis')
+
+        ax.coords[0].set_axislabel('Pixel x-axis')


As per comments above, I would just do X [pixels] and Y [pixels].

Or X / pixels, Y / pixels ;-)

wtgee · 2018-01-12T04:18:42Z

pocs/utils/images/__init__.py

@@ -103,14 +104,22 @@ def _make_pretty_from_fits(fname, **kwargs):
        ax = plt.subplot(projection=wcs)

        ax.coords.grid(True, color='white', ls='solid')


This line could be moved below if clause so it is not repeated.

AnthonyHorton · 2018-01-12T04:48:59Z

pocs/utils/images/__init__.py

+        dec.set_axislabel('Declination')
+
+        ra.set_major_formatter('hh:mm:ss')
+        dec.set_major_formatter('dd:mm:ss')


For Huntsman, PANOPTES or in fact most other optical instruments the field of view will be large enough that including seconds in the major tick labels is unnecessary. hh:mm and dd:mm is probably better.

AnthonyHorton · 2018-01-12T04:51:12Z

pocs/utils/images/__init__.py


    new_filename = fname.replace('.fits', '.jpg')

    data = getdata(fname)
-    plt.imshow(data, cmap='cubehelix_r', origin='lower')
+
+    norm = ImageNormalize(interval=MinMaxInterval(), stretch=LogStretch())


Might be better to set the interval to trim off a (very) small percentage at either end, so that the normalisation doesn't get driven by dead pixels, hot pixels, cosmic rays and saturated stars.

What sort of percentage are we thinking? Something like 0.1%?

*for lines with (?) contents should be added to config? *change from minmax to percentile interval *add alpha option for grid (transparent lines)

wtgee · 2018-01-12T13:21:50Z

pocs/utils/images/__init__.py

    wcs = WCS(fname)
+    ax = plt.subplot(projection=wcs)


This line might need to stay inside the if. I'm not sure what happens if you pass a projection but don't have a valid WCS. Have you tried on the original images that weren't solved yet?

Just for fun I just ran it on an image that a PANOPTES unit just took that has a pretty empty star field and a meteor streak. Looks like it handles the lack of wcs fine.

There's a lot of dead white space in there around the edges. I think you can do a fig.tight_layout() to get rid of some of that.

I tested it on my Jupyter notebook and it seems to work just fine. If I don't assign anything to projection then I can't use coords[] for the pixel coordinates. I would have to use this for else
ax = plt.subplot()
ax.set_xlabel('X / pixels')
ax.set_xlabel('Y / pixels')

But I think what I have should work just fine :)
If not then we can change it to this.

Awesome, image looks good, is this with percent_value = 99.81? Only asking because maybe we should make that value able to be modified since when I was trying different percentages, and even changing it by 0.1% it changes quite drastically.

And I will add the fig.tight_layout() soon and let you know how it looks.

wtgee · 2018-01-12T13:29:04Z

This has nothing to do with your code as it was already in the function, but we shouldn't actually be doing the symlink stuff here since we don't do it in _make_pretty_from_cr2. Can you remove all those lines since you are updating the function anyway? Thanks!

wtgee · 2018-01-12T13:30:51Z

pocs/utils/images/__init__.py

@@ -87,12 +90,34 @@ def make_pretty_image(fname, timeout=15, **kwargs):  # pragma: no cover
 def _make_pretty_from_fits(fname, **kwargs):
    config = load_config()

-    title = '{} {}'.format(kwargs.get('title', ''), current_time().isot)
+    title = '{} {}'.format(kwargs.get('title', ''), current_time(pretty=True).isot)


Tests are failing on this. Doing the pretty=True turns it into a string, so then you can't call .isot on it. You want one or the other. They are basically the same except pretty has spaces for easier reading.

wtgee · 2018-01-12T23:39:41Z

Yes, that image is from using your branch directly (I removed the isot thing to make it work). So yes, using that same percentage. It's to some extent going to depend on each image but we probably want to find something that works most of the time and then pass an option via kwargs.

…

On 13 Jan 2018 10:36 AM, "jermainegug" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pocs/utils/images/__init__.py <#319 (comment)>: > wcs = WCS(fname) + ax = plt.subplot(projection=wcs) I tested it on my Jupyter notebook and it seems to work just fine. If I don't assign anything to projection then I can't use coords[] for the pixel coordinates. I would have to use this for else ax.set_xlabel('X / pixels') ax.set_xlabel('X / pixels')``` But I think what I have should work just fine :) If not then we can change it to this. Awesome, image looks good, is this with `percent_value = 99.81`? Only asking because maybe we should make that value able to be modified since when I was trying different percentages, and even changing it by 0.1% it changes quite drastically. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#319 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEUUMndOSrI4n8AeKUaKsgZGyAw8t83ks5tJ-xhgaJpZM4RbqTp> .

jermainegug · 2018-01-12T23:45:37Z

I used the example image and the wcs is working great! Like @AnthonyHorton mentioned, we wont need the units in the axis label since they are written to values on the image.

wtgee · 2018-01-13T03:37:21Z

Note that you are failing on some codestyle issues. See https://travis-ci.org/panoptes/POCS/builds/328330892#L4924

With atom or sublime you can use an autopep8 tool that should correct most things.

jermainegug · 2018-01-13T12:51:49Z

autopep8 doesn't change anything in my file..

wtgee · 2018-01-13T19:49:49Z

autopep8 doesn't change anything in my file..

They were related to the comments you deleted anyway.

Can you post a pic with the tight_layout? Also, in pick above I see the title but not a date. Seems like there should be both from this code.

jermainegug · 2018-01-15T00:17:51Z

Check seems to give us this:

pocs/tests/utils/test_image_utils.py::test_clean_observation_dir
  /home/travis/build/panoptes/POCS/pocs/utils/images/__init__.py:140: 
UserWarning: Can't link latest image: [Errno 2] No such file or directory: 
'/tmp/tmppbxfbjvv/solved.jpg' -> '/var/panoptes/images/latest.jpg'
    warn("Can't link latest image: {}".format(e))

wtgee · 2018-01-15T00:22:05Z

See my comment above about removing all the link stuff: #319 (comment)

jermainegug · 2018-01-15T00:30:31Z

Oh yes, I forgot to delete those. Thanks.

wtgee · 2018-01-15T00:50:34Z

Oh yes, I forgot to delete those. Thanks.

There is other stuff that goes with it. For instance once you delete that I don't think you need the config or the image_dir. Make sure to do a full cleanup please. Thanks!

wtgee · 2018-01-15T00:58:58Z

Regarding the title and time from the fits headers, yes I would think so. The example I showed up lets a title in the kwargs override that so it would just be a backup.

*fix getdata

jermainegug · 2018-01-15T01:58:13Z

Ignore the recent commit.

wtgee · 2018-01-15T01:59:51Z

You want to keep the kwargs. That lets someone pass something in to override. So if I am using make_pretty_image interactively from a jupyter console (which I do freqently) I can say make_pretty_image(title="test_01") and have it work. That should take precedence over the fits header because I am explicitly asking for it.

wtgee · 2018-01-15T02:00:09Z

Ignore the recent commit.

Too late. :)

(?) I am not sure if I should use the imported current time or use the DATE-OBS from the header.

*change title

wtgee · 2018-01-15T03:35:36Z

pocs/utils/images/__init__.py


-    image_dir = config['directories']['images']
+    percent_value = 99.9


Just to make it flexible for the future I would pull this from kwargs too. So kwargs.get('normalize_clip_percent', 99.9)

wtgee · 2018-01-15T03:36:27Z

pocs/utils/images/__init__.py

+        ax.set_xlabel('X / pixels')
+        ax.set_ylabel('Y / pixels')
+
+    ax.imshow(data, norm=norm, cmap='inferno', origin='lower')


Also make this slightly more flexible by doing a cmap=kwargs.get('cmap', 'inferno'). Then you can play with different colormaps in runtime without touching code.

*allow for selection of percent, color map *neaten up code

jermainegug · 2018-01-15T04:08:04Z

Yay! All checks passed :)

wtgee

One more small change and then I think it's good.

wtgee · 2018-01-15T22:02:52Z

pocs/utils/images/__init__.py


-    title = '{} {}'.format(kwargs.get('title', ''), current_time().isot)
+    title = kwargs.get('title', header.get('FIELD', ''))
+    date_time = header.get('DATE-OBS', current_time()).replace('T', ' ', 1)


This might need to be tested. current_time will return a time object and then the replace will probably fail. If you leave at current_time(pretty=True) it will return a string and the replace will work on a string (it just won't replace anything since the T has already been removed, but that's fine), which is probably safer and means you don't need to bother testing that.

I have changed it now but I will test anyways (want to make sure that it doesn't get rid of the T in UTC).

Oh, current_time doesn't print out UTC. So it is all good then. Works just fine.

wtgee

LGTM! We've beat this PR to death so any additional items can be fixed via an Issue.

@jermainegug I'll let you do the honors of merging your own PR. Make sure to select the Squash option and to say "Fixes #232" in merge commit. You are also okay to delete the branch after (it will show you a button). Thanks!

jermainegug · 2018-01-16T21:21:42Z

Indeed we have!

No problem but I do not have write access so the merge button does not show up.

Not sure if you can do this but did you want me to say it also fixes #43 along with #232 ?
And I guess we can close AstroHuntsman#2

wtgee · 2018-01-16T21:44:00Z

Ok, I'll go ahead and merge. #43 stays open as it is more of a question of what we want to do with the pretty pictures than it is about getting pretty pictures working.

add make_pretty_from_fits file

4eb018d

*creates norm variable *checks wcs (issue) *changed cmap from cubealpha_r to inferno

edit _make_pretty_from_fits

210cdc0

*using the function in make_pretty_from_fits file *delete make_pretty_from_fits file

wtgee reviewed Jan 12, 2018

View reviewed changes

edit check for wcs

c1f7c01

*fixed check for wcs *not sure about RA & dec axes

RA & dec fix (?)

a6175f5

*just set axis labels to RA and dec

make object-oriented(?)

46b93c7

*descriptive axes *coordinate system in title? *how to get equinox?

wtgee reviewed Jan 12, 2018

View reviewed changes

AnthonyHorton reviewed Jan 12, 2018

View reviewed changes

jermainegug added 2 commits January 12, 2018 16:43

*edit for units in axis labels

723c7bf

edit and fix _make_pretty_images_from_fits

49c2cf7

*for lines with (?) contents should be added to config? *change from minmax to percentile interval *add alpha option for grid (transparent lines)

wtgee reviewed Jan 12, 2018

View reviewed changes

jermainegug added 2 commits January 13, 2018 10:46

*get rid of .isot for current time

f5aa627

*don't need units on wcs image axis

0e491aa

jermainegug added 2 commits January 13, 2018 22:43

*add tight_layout()

fbbd86a

*delete comments

da5bf9b

*remove symlink lines

812e417

*cleanup unused code

99b074a

jermainegug added 2 commits January 15, 2018 12:55

*fix title to show field and datetime from fits header

ba85b33

*fix getdata

*take away **kwargs

a98f106

jermainegug added 4 commits January 15, 2018 13:04

Use fits header items as title and use kwargs as back up

8d2d647

*Got rid of current_time

e0a83fa

(?) I am not sure if I should use the imported current time or use the DATE-OBS from the header.

*import fits

093e49e

*import getheader

c47e93d

*change title

wtgee reviewed Jan 15, 2018

View reviewed changes

Edit to be more generic

e4bf678

*allow for selection of percent, color map *neaten up code

wtgee mentioned this pull request Jan 15, 2018

Fix make_pretty_image for FITS files #232

Closed

wtgee reviewed Jan 15, 2018

View reviewed changes

*pretty=True for current_time

3c7ebf9

jermainegug mentioned this pull request Jan 16, 2018

create pretty images from FITS AstroHuntsman/POCS#2

Closed

wtgee approved these changes Jan 16, 2018

View reviewed changes

wtgee merged commit 46d7ae8 into panoptes:develop Jan 16, 2018

jermainegug deleted the make-pretty-from-fits branch January 16, 2018 22:09

wtgee mentioned this pull request Jan 24, 2018

Release 0.6.1 #415

Closed

		@@ -103,14 +104,22 @@ def _make_pretty_from_fits(fname, **kwargs):
		ax = plt.subplot(projection=wcs)

		ax.coords.grid(True, color='white', ls='solid')


		image_dir = config['directories']['images']
		percent_value = 99.9

add make_pretty_from_fits file to develop #319

add make_pretty_from_fits file to develop #319

Conversation

jermainegug commented Jan 12, 2018 • edited Loading

wtgee commented Jan 12, 2018

wtgee commented Jan 12, 2018

AnthonyHorton commented Jan 12, 2018

wtgee commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyHorton Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 12, 2018 • edited Loading

Codecov Report

AnthonyHorton commented Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jermainegug Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jermainegug Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgee commented Jan 12, 2018

Choose a reason for hiding this comment

wtgee commented Jan 12, 2018 via email

jermainegug commented Jan 12, 2018

wtgee commented Jan 13, 2018

jermainegug commented Jan 13, 2018

wtgee commented Jan 13, 2018

jermainegug commented Jan 15, 2018 • edited Loading

wtgee commented Jan 15, 2018

jermainegug commented Jan 15, 2018

wtgee commented Jan 15, 2018

wtgee commented Jan 15, 2018

jermainegug commented Jan 15, 2018

wtgee commented Jan 15, 2018

wtgee commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jermainegug commented Jan 15, 2018

wtgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jermainegug Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

wtgee left a comment

Choose a reason for hiding this comment

jermainegug commented Jan 16, 2018 • edited Loading

wtgee commented Jan 16, 2018

jermainegug commented Jan 12, 2018 •

edited

Loading

AnthonyHorton Jan 12, 2018 •

edited

Loading

codecov bot commented Jan 12, 2018 •

edited

Loading

AnthonyHorton commented Jan 12, 2018 •

edited

Loading

jermainegug Jan 12, 2018 •

edited

Loading

jermainegug Jan 12, 2018 •

edited

Loading

jermainegug commented Jan 15, 2018 •

edited

Loading

jermainegug Jan 15, 2018 •

edited

Loading

jermainegug commented Jan 16, 2018 •

edited

Loading