Improve error message for scaled units in FITS

[taldcroft]

Issue #1976 highlighted some confusion when trying to write (to FITS) a table with scaled units. In this case it was a field with % as the unit. The error message in this case is:

ERROR: ValueError: The FITS unit format is not able to represent scale. Multiply your data by 1.000000e-02. [astropy.units.format.fits]

This is essentially correct, but misses a lot of the information a user actually needs to understand what is happening and how to fix it. This is particularly the case for users without a deep understanding of units and what scale means in that context.

Suggest improving the error message to include:

• Name of the column
• Unit of the column and scale factor
• Pointer to relevant documentation

Marked as Easy and milestoned for 0.3.1.

[krngrvr09]

Hey I am new to open source development, and am very excited about working with astropy. Since, I might not be qualified enough to fix most of the issues, this seems relatively easy. I have been doing research for about 2 days now, and I cannot seem to figure it out. Can you suggest me how to go about this. I am willing to learn anything i don't yet know. Thank you!

[mhvk]

@krngrvr09 - welcome! This particular error message is generated in astropy/units/format/fits.py, near the very end (in the to_string method). I actually do not think it is that easy to provide everything that @taldcroft listed at this location, since in that routine all you have is the actual unit that should be transformed -- you do not have access to the Column name. To have that would require catching the exception higher up, which I do not think is quite worthwhile. Still, I do think the error message could be more descriptive, even just by listing the unit involved. E.g., one could think of an error message like

"The FITS unit format is not able to represent scale, and hence cannot represent the unit '{0}'. If you need to store data with this unit, multiply them first by {0.scale:e}".format(unit)

I think it is also a good idea to include a pointer to documentation, but am actually not sure where to find this (does it exist?). So, that might be part of the PR...

[astrofrog]

@mhvk - I was chatting with @krngrvr09 on IRC and we thought it would be handy to have the exception be raised in astropy/io/fits/connect.py instead, so as to have access to the column name (and it's easy to check if the scale is 1 or not there).

[taldcroft]

@mhvk - the low level error message is not terrible, since normally a user would hit this when specifically doing someunit.to_string(format='fits'). Handling (or preventing) the exception that gets raised when writing a FITS file via the Table connector should be done in astropy/io/fits/connect.py as @astrofrog said and I think this has value. Basically when a low-level routine might throw an exception in a common case for a high-level use, the exception should be caught / prevented at the higher level.

[krngrvr09]

I have been trying to understand the codebase of astropy.
So is, the unit being "%", the only reason why we get this error. If yes, or if we dont know yet, should I just check for unit being "%" in connect.py and not the scale?
because it is only after calling 'decompose_to_known_units' function, I see the scale changing from 1.0 to 0.01.

I have been successfully able to pass the column name, units, scale and link to the documentation in the error raised. The only question remains, what should I check for?
What do you think?
Please correct me if I'm wrong somewhere. Thank You.

[mhvk]

@krngrvr09 - this exception will be raised for any unit which has a scale that is not just given by a prefix; so, e.g., Unit(0.01*u.m/u.s).to_string('fits') fails as well (but Unit(u.cm/u.s).to_string('fits') works).

Generally, I do not think you should be checking whether a unit may fail higher up -- the check should only happen in one location (after all, the fits standard may evolve or we manage to solve it another way; e.g., the example above we could already solve within the fits standard). Instead, I think it is better to try to catch the exception, as in:

try:
    unit_string = unit.to_string('fits')
except ValueError as exc:
   [add something more useful to the exception text raised in format/fits.py]
   raise ValueError(new_text)

(Note: I have not looked how the code higher up actually works and thus do not know how practical this is.)

[taldcroft]

@krngrvr09 - following on from what @mhvk said, I've outlined a couple of key elements for a solution to get you going in the right direction. I made a local branch and edited entirely via the web interface (nice!). You can see the lines at:

https://github.com/taldcroft/astropy/compare/scale-error-demo#diff-0

The idea is to make a specific exception class in the low level routine, and catch only that error in the high level one.

[taldcroft]

Note also this is purposely rough and might have errors! :-)

[krngrvr09]

Alright, so finally, I have made all the required changes, and pushed it to my forked repository. I want you all to review it once, and suggest any changes.

https://github.com/krngrvr09/astropy/compare/astropy:master...master

Should I send the pull request?

[taldcroft]

@krngrvr09 - looking at that branch it looks mostly OK except for some minor issues:

• Somehow setuptools.0.9.8.tar.gz ended up in the repo
• The nitpicky = True statement should not be here. If there are really 2000 broken links then that needs to be a separate issue. It's important that changes in a PR stay within the scope of the issue it's fixing.
• There are a large number of "noise" commits which look like learning the process a bit. That's fine, but we don't want to have all that getting back into master.

For the final issue you can try squashing with git, or else just start over with a new branch (based off the most recent master) and make the changes over again. Sometimes the manual approach ends up being faster.

[taldcroft]

One more thing is that you will eventually need to add a test case which shows that the new error handling works as expected.

[astrofrog]

Note: we need to make sure that the tar file doesn't end up in the final commits for this PR.

[krngrvr09]

Everything done.
https://github.com/krngrvr09/astropy/compare/astropy:master...master

just need to add the test case.

[taldcroft]

Looks good enough for a pull request.

[krngrvr09]

I have been searching the net, but couldn't find any valuable resource. Can anyone guide me on how to write test cases for this particular fix. Although, I am well acquainted with python, it seems necessary to take help, when working with such a big project, for the first time.

Thank You!

[pllim]

Maybe something like this might work for you?

def test_something():
    try:
        # do something that raise your particular error here
    except YourNewError as e:
        assert 'your text' is in str(e)

[taldcroft]

Another standard option is to use pytest.raises. There is a very brief introduction here:
http://astropy.readthedocs.org/en/latest/development/testguide.html#using-py-test-helper-functions

So I think you'll want to make a Table object that has the unit as % and then try to write it.

[embray]

In general use raises, but if you want to actually inspect the message returned with the exception @pllim's suggestion is fine too.

[astrofrog]

The standard in most of the rest of astropy is:

with pytest.raises(SomeException) as exc:
    assert str(exc) == 'whatever'

[astrofrog]

We should probably describe this in the developer docs btw.

[krngrvr09]

Thank you everyone for your advice.
Alright, test case is ready. I was wondering if we should have a data folder for 'ipac' tables as well.
What do you think?

[mwcraig]

@astrofrog -- there is a mention of testing, including how to check for an error in the rearrangement at #1712 , though it is a little buried. You can find it in the worked example of contributing code.

[embray]

@astrofrog See I didn't even know you could do that. Or if I did I forgot :)

[astrofrog]

Actually I usually do:

with pytest.raises(SomeException) as exc:
    assert exc.value.args[0] == 'whatever'

because I think that str(exc) may not always work.

[wkerzendorf]

@embray @taldcroft @astrofrog Why is this issue still open? #2023 was merged and at least seems to fix it (looking at the name not at the code). One of the GSoC students intends to work on this and wants to see if this is still viable.

[embray]

Actually I'm not sure. I feel like maybe #2023 didn't fully implement the desired fix, but reading through things I'm not sure. I'll just close it for now, but if anyone thinks the current fix is insufficient they can open a new issue I guess.