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

Added wrapper class for astropy.core.Time, TaggedTime #198

Merged
merged 2 commits into from
May 20, 2016
Merged

Added wrapper class for astropy.core.Time, TaggedTime #198

merged 2 commits into from
May 20, 2016

Conversation

bernie-simon
Copy link
Contributor

When the asdf validator compares tree nodes against the schema and the
schema contains a tag field, it requres that the node contain the same
tag, or alse the validation fails. Asdf handles this by creating nodes
of type Tagged and its subtypes, all which contain a _tag field that
holds the value matched against the schema. However, only a limited
number of subtypes have been implemented: TaggedDict, TaggedList, and
TaggedString. This commit adds a new type, TaggedTime, to support time
values contained in a tree node.

When the asdf validator compares tree nodes against the schema and the
schema contains a tag field, it requres that the node contain the same
tag, or alse the validation fails. Asdf handles this by creating nodes
of type Tagged and its subtypes, all which contain a _tag field that
holds the value matched against the schema. However, only a limited
number of subtypes have been implemented: TaggedDict, TaggedList, and
TaggedString. This commit adds a new type, TaggedTime, to support time
values contained in a tree node.
@nden
Copy link
Contributor

nden commented May 18, 2016

I'm not sure I understand this. The module docs say that this is intended to create tags for basic types. Time should already be a tag. Could you post a failing example or an example of what you are trying to achieve and also turn it into a test.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 93.585% when pulling d794c6c on bernie-simon:master into afecd5c on spacetelescope:master.

@bernie-simon
Copy link
Contributor Author

On May 18, 2016, at 3:05 PM, Nadia Dencheva notifications@github.com wrote:

I'm not sure I understand this. The module docs say that this is intended to create tags for basic types. Time should already be a tag. Could you post a failing example or an example of what you are trying to achieve and also turn it into a test.

Here is the problem. Models validates changes to image metadata in setattr in properties.py. The validation process expects that where there is a tag in the schema the metatdata value that was changed also has a field containing the tag string. In the recent past, this was not handled properly, so any tags in the core schema would cause Models to throw an error. In particular, there is a tag in the core schema for Time, so setting the value of a time field, which happens in model_base.py when an image is updated, would throw an error.

The fix is to tag the metadata being updated, which means to wrap it in a tagged type, which has the field that the validator is looking for. That code was added to _cast in properties.py and covers all the cases in properties.py where the validator is called.

    tag = schema.get('tag')
    if tag is not None:
        val = tagged.tag_object(tag, val)

So far, so good. But this only fixes Models, there is a new problem in asdf, because it can only handle a limited number of types in tag_object: strings, lists, and dictionaries. The time field is an astropy time object. So I had to write some new code to handle time objects for tag_object:

elif isinstance(instance, time.Time):
    return TaggedTime(instance, tag)

class TaggedTime(Tagged, time.Time):
"""
An Astropy time object with a tag attached.
"""
def new(cls, instance, tag):
self = time.Time.new(type(instance), instance)
self._tag = tag
return self

Without this fix, tag_object throws an error:

TypeError: Don't know how to tag a <class 'astropy.time.core.Time�>

This does not create a new dependency for asdf, as the astropy time class has serialization code in asdf/asdf/tags/time.py

I hope this clarifies the issue

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 93.585% when pulling c2f72c0 on bernie-simon:master into afecd5c on spacetelescope:master.

@bernie-simon bernie-simon merged commit bb32b04 into asdf-format:master May 20, 2016
@nden
Copy link
Contributor

nden commented May 25, 2016

@bernie-simon Could you add a Changelog entry under 1.1.0. Also please open an issue to add a test. If it makes sense, the test can be added to the models package with a reference to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants