Improved error message for scale units in FITS. #2023

krngrvr09 · 2014-01-30T21:54:54Z

Fix for issue #1979.

Improved the error message and added a test case.
Please review the changes.

embray · 2014-01-30T22:09:18Z

astropy/io/fits/tests/test_connect.py

+        table=Table.read(infile, format='ipac')
+        table.write('table.fits',format='fits', overwrite=True)
+        out = table.vstack([t1, t2, t4], join_type='outer')
+	    assert exc.__class__.__name__ == 'UnitScaleError'


The indentation is inconsistent here.

astrofrog · 2014-01-31T13:20:37Z

@krngrvr09 - it looks like the tests are failing. You can run the test suite locally bby doing:

python setup.py test

do you get it to pass on your computer?

mdboom · 2014-01-31T16:15:59Z

astropy/io/fits/connect.py

+                        "The column ' {0} ' could not be stored in FITS format "
+                        "because it has a scale ' ({1}) ' that "
+                        "is not recognized by the FITS standard. Either scale "
+                        "the data or change the units.".format(col.name, str(scale)))


This message looks great. Only one minor nitpick: I don't think we need the extra spaces inside the single quotes, i.e.: '{0}' is fine, and I don't think we need the extra single quotes around {1}, i.e.: ({1}) is fine.

I usually don't make the quotes part of the format string at all and just use {0!r}; then Python will use the appropriate quotes around the string.

The problem with {0!r} is that you can get u prefixes on Python 2. Not terrible, but a little user unfriendly.

Ah, true. That's why I also sometimes do repr(foo).lstrip('bu') but that's probably almost uglier :) It's better practice in general in case the string you're inserting contains quotes itself but that isn't really an issue here I guess.

krngrvr09 · 2014-02-02T11:32:37Z

@astrofrog Yes, the tests were failing on my system too. I have fixed them.
@mdboom I agree, the if-statement, is necessary. I have added that too.

Hopefully everything seems good now.
If there are more changes to be made, let me know. Thank you.

astrofrog · 2014-02-02T11:38:35Z

astropy/io/fits/connect.py

+            try:
+                col.unit = col_unit.to_string(format='fits')
+            except UnitScaleError:
+                raise UnitScaleError(


Just a couple of minor esthetic changes. You only use col_unit once, so I think you could just leave:

col.unit = input[col.name].unit.to_string(format='fits')

since you don't gain anything from defining the intermediate variable col_unit.

For scale, it does make sense to define it because it does help the readability in the exception, but you can move the

scale = ...

line inside the except clause, because it's only needed then. That is:

except: scale = ... raise UnitScaleError(...)

That had actually been my suggestion in the first place; earlier the exception message I suggested used the unit name in the message, so it was used twice. But now I agree it's not needed more than once.

astrofrog · 2014-02-02T11:42:38Z

@krngrvr09 - this is almost ready, I left a few comments above.

krngrvr09 · 2014-02-02T13:16:57Z

@astrofrog I have deleted the line out=table.vstack.... And also made the required changes in test_connect.py and made the changes in connect.py.

astrofrog · 2014-02-02T13:33:56Z

astropy/io/fits/tests/test_connect.py

+    c = ['x', 'y', 'z']
+    t = Table([a, b, c], names=('a', 'b', 'c'), meta={'name': 'first table'})
+    t['a'].unit='percent'
+    t.format='ipac'


Just saw this line, which is not needed (t.format='ipac') - can you remove it?

krngrvr09 · 2014-02-02T15:13:07Z

@astrofrog Done.

astrofrog · 2014-02-02T15:22:04Z

@krngrvr09 - this is looking good. It looks like there is a conflict in the changes compared to the latest version of Astropy, so you will need to rebase on the latest master of the main repository. Have you done this before? There are instructions provided here:

http://docs.astropy.org/en/stable/development/workflow/development_workflow.html#rebase-on-trunk

but if you have any issues it might be easier to chat about it live on IRC. I am not available right now, but you could go on during the week and ask if you have any issues. In any case, if you do mess up, then don't open a new pull request, because there is always a way we can recover.

mdboom · 2014-02-07T18:54:03Z

astropy/io/fits/connect.py

+-------
+is_fits : bool
+Returns `True` if the given file is a FITS file.
+"""


It looks like this docstring has lost its indentation here. Each of these lines should have 4 spaces at the beginning.

krngrvr09 · 2014-02-08T18:08:21Z

@keflavich I have made the required changes. I hope, everything is now, Ok. But still if there are any other changes, to be made. Please suggest.

@astrofrog I have been able to do the rebase. Apologies for the noise commits. Is there anything else, I have to do?

taldcroft · 2014-02-10T12:43:06Z

@krngrvr09 - please look at the file diffs and eliminate any diffs that are not specifically related to the patch you are applying. There are still a number of diffs that are just noise and which incorrectly change the formatting.

At this point the number of commits has gone over 50 during your learning process, so I think it's time for you to start over from scratch from the current master and apply your patch cleanly in a few commits. It's not too hard when you look at the file diffs.

taldcroft · 2014-02-10T21:11:55Z

@krngrvr09 - by "start over" I mean:

git branch -m fits fits-backup
git checkout master
git pull upstream master # where upstream is the main astropy/astropy repo
git checkout -b fits

Now based on the file diffs you currently see here, edit the appropriate files to replicate those diffs. Run tests locally to make sure everything works. Then:

git diff master

Make sure that the only changes are ones related to your fix, and make sure that all the diffs you see in this branch currently on github are in your new patch. No extra whitespace or formatting changes.

Finally, commit your changes and do a force push to github:

git commit -am "Improve error message for scale units in FITS"
git push -f origin fits

BTW, you could also squash all the 50 commits into 1. You can google on "git squash" to find some tutorials on how to do this. This will also require a force push to github.

krngrvr09 · 2014-02-12T05:22:51Z

@taldcroft Thank you very much for your help. Honestly, I think, what we just did was very cool!

mdboom · 2014-02-12T14:47:25Z

Look good to me!

Improved error message for scale units in FITS.

taldcroft · 2014-02-12T16:46:47Z

@krngrvr09 - merged, thanks!

krngrvr09 · 2014-02-13T05:08:35Z

Thanks to the AstroPy community. I learned a lot!

Improved error message for scale units in FITS. Conflicts: astropy/io/fits/connect.py astropy/io/fits/tests/test_connect.py

embray mentioned this pull request Jan 30, 2014

Fixed Issue #1979 #2015

Closed

embray reviewed Jan 30, 2014
View reviewed changes

mdboom reviewed Jan 31, 2014
View reviewed changes

astrofrog reviewed Feb 2, 2014
View reviewed changes

mdboom reviewed Feb 7, 2014
View reviewed changes

Improve error message for scale units in FITS

a919550

taldcroft added a commit that referenced this pull request Feb 12, 2014

Merge pull request #2023 from krngrvr09/fits

ba9b2a4

Improved error message for scale units in FITS.

taldcroft merged commit ba9b2a4 into astropy:master Feb 12, 2014

embray added this to the v0.3.1 milestone Feb 12, 2014

embray added the units label Feb 12, 2014

embray added Bug and removed Bug labels Feb 12, 2014

taldcroft added a commit that referenced this pull request Feb 25, 2014

Merge pull request #2023 from krngrvr09/fits

9c00b01

Improved error message for scale units in FITS. Conflicts: astropy/io/fits/connect.py astropy/io/fits/tests/test_connect.py

wkerzendorf mentioned this pull request Mar 12, 2014

Improve error message for scaled units in FITS #1979

Closed

krngrvr09 deleted the fits branch March 12, 2014 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved error message for scale units in FITS. #2023

Improved error message for scale units in FITS. #2023

krngrvr09 commented Jan 30, 2014

embray Jan 30, 2014

astrofrog commented Jan 31, 2014

mdboom Jan 31, 2014

embray Jan 31, 2014

mdboom Jan 31, 2014

embray Jan 31, 2014

krngrvr09 commented Feb 2, 2014

astrofrog Feb 2, 2014

embray Feb 3, 2014

astrofrog commented Feb 2, 2014

krngrvr09 commented Feb 2, 2014

astrofrog Feb 2, 2014

krngrvr09 commented Feb 2, 2014

astrofrog commented Feb 2, 2014

mdboom Feb 7, 2014

krngrvr09 commented Feb 8, 2014

taldcroft commented Feb 10, 2014

taldcroft commented Feb 10, 2014

krngrvr09 commented Feb 12, 2014

mdboom commented Feb 12, 2014

taldcroft commented Feb 12, 2014

krngrvr09 commented Feb 13, 2014

Improved error message for scale units in FITS. #2023

Improved error message for scale units in FITS. #2023

Conversation

krngrvr09 commented Jan 30, 2014

Choose a reason for hiding this comment

astrofrog commented Jan 31, 2014

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

krngrvr09 commented Feb 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Feb 2, 2014

krngrvr09 commented Feb 2, 2014

Choose a reason for hiding this comment

krngrvr09 commented Feb 2, 2014

astrofrog commented Feb 2, 2014

Choose a reason for hiding this comment

krngrvr09 commented Feb 8, 2014

taldcroft commented Feb 10, 2014

taldcroft commented Feb 10, 2014

krngrvr09 commented Feb 12, 2014

mdboom commented Feb 12, 2014

taldcroft commented Feb 12, 2014

krngrvr09 commented Feb 13, 2014