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

logging with no add_keywords #68

Closed
crawfordsm opened this issue May 6, 2014 · 8 comments
Closed

logging with no add_keywords #68

crawfordsm opened this issue May 6, 2014 · 8 comments
Assignees
Milestone

Comments

@crawfordsm
Copy link
Member

Logging does not seem to behave as expected in the api. In the api, we suggest that the if add_keyword is not specified, the task name and parameters should be added to meta, but that does not seem like the case right now.

@crawfordsm crawfordsm added this to the 0.1 milestone May 6, 2014
@mwcraig
Copy link
Member

mwcraig commented May 7, 2014

My understanding of the logging discussion was that the preference was for not automatically adding logging information if the user didn't supply it.

If we want to add this in, I can -- it would only require changing what value the decorator provides as the default.

My personal preference would be to have some sort of logging built in, with the ability for the user determined to leave no trace of what they've done to opt out by calling with add_keywords=None.

@crawfordsm
Copy link
Member Author

I would agree. Adding an extra keyword to the header shouldn't break anything and I think it enhances the data product as you can track what you have done to it.

Logging wasn't entirely intuitive to me, so I would highlight it as something that needs to have plenty of examples in the documentation.

Although I do like the non-intrusiveness of it as well.

@mwcraig
Copy link
Member

mwcraig commented May 14, 2014

Tried implementing this morning hit one (at least) roadblock: what is the sensible value to put in for an argument that is a CCDData object? Options I see, and their downsides:

  • String representation of CCDData: too long to be representable in FITS; not particularly meaningful even if it were.
  • The repr of CCData has the same problems.

Apparently I'm not so creative this morning because that is all I came up with.

To be clear, there is no technical problem with coming up with the parameter values, and one easy out is the add the __name__ of the function called but not the parameters.

@mwcraig
Copy link
Member

mwcraig commented May 20, 2014

Will simply skip any numpy.ndarray, NDData and CCDData arguments.

@crawfordsm
Copy link
Member Author

For right now, I think that's the way to go until we see how it works

@mwcraig
Copy link
Member

mwcraig commented May 21, 2014

I hit another wall here...this time the problem is FITS. There are two FITS conventions that come into play:

  • HIERARCH, which allows keyword names longer than 8 characters (e.g. a function name like subtract_overscan)
  • CONTINUE, which allows string values longer than 68 characters (the maximum length in FITS after accounting for keyword name length, equal sign and enclosing single quote) (e.g. a nicely formatted argument list)

The problem: astropy.io.fits does not handle the case of HIERARCH and CONTINUE used together for a single keyword. A little bit of hacking on the source of card.py makes it look like the fix would be a few lines of code...but maybe more, and I have no idea if such a beast would be readable by anything other than astropy.

As it stands an error gets raised if you try (a really simple hack to card.py--just passing instead of raising an error--mangles the keyword name).

Alternatives:

  1. Construct a shortened keyword based on the function name, perhaps with the shortened name saved as a keyword?
  2. Record each argument as a key/value pairs, numbered (e.g. subtract_overscan01, subtract_overscan02)

@crawfordsm -- I'll give this another try after moving away from fits Header as the metadata (turns out that isn't completely case-insensitive anyway...HIERARCH keywords are case sensitive).

Then all the FITS stuff can be refactored into the .to_hdu method of CCDData and decoupled from the logging itself.

@crawfordsm
Copy link
Member Author

I would avoid messing with io.fits as we don't want to make files which aren't FITS compliant.

Another option would be that we put in these keywords but any keyword is stripped out before they are written out to FITS. Not ideal really.

I think my preferred option would be to have a default 8-letter or less keyword. This could also be set to None if people want to turn off logging.

@mwcraig
Copy link
Member

mwcraig commented May 27, 2014

Closed by #89

@mwcraig mwcraig closed this as completed May 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants