Skip to content

Support circular references in ASDF-serialized models#10189

Merged
nden merged 4 commits intoastropy:masterfrom
eslavich:eslavich-fix-serialization-recursive-inverses
Apr 24, 2020
Merged

Support circular references in ASDF-serialized models#10189
nden merged 4 commits intoastropy:masterfrom
eslavich:eslavich-fix-serialization-recursive-inverses

Conversation

@eslavich
Copy link
Contributor

Description

This pull request updates the minimum asdf version to 2.6.0, and uses a new asdf feature (generators returned from ExtensionType.from_tree_tagged methods) to enable handling of models with circular user inverses.

I'm also removing explicit calls to asdf.yamlutil.custom_tree_to_tagged_tree and asdf.yamlutil.custom_tree_to_tagged_tree as those are no longer necessary.

@astropy-bot astropy-bot bot added io.misc zzz 💤 io.misc.asdf archived: Go to asdf-astropy repo labels Apr 23, 2020
@bsipocz bsipocz requested a review from nden April 23, 2020 04:56
@bsipocz bsipocz added this to the v4.1 milestone Apr 23, 2020
@bsipocz
Copy link
Member

bsipocz commented Apr 23, 2020

note to reviewer: check the travis status on the second to last commit

@eslavich
Copy link
Contributor Author

The last batch of failed checks were due to the GitHub outage, which seems to have been resolved. Is it possible to re-run them?

@pllim
Copy link
Member

pllim commented Apr 23, 2020

Restarted CI

Copy link
Contributor

@nden nden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I left a couple minor comments.

CHANGES.rst Outdated
- Added serialization of parameter constraints fixed and bounds. [#10082]

- Fix ASDF serialization of circular model inverses, and remove explicit calls
to ``asdf.yamlutil`` functions that because unnecssary in asdf 2.6.0. [#10189]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because they are unnecessary ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thanks, I intended to write "that became unnecessary" but went off the rails.

new_mapping.append(i)
transform = transform & ConstantType.from_tree(
{'value': int(entry.value)}, ctx)
transform = transform & Const1D(int(entry.value))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting to int is not necessary here. I'm guessing this was the idea for the old Const model but in modeling all parameters are turned into floats.

@nden nden merged commit 0aedb46 into astropy:master Apr 24, 2020
@eslavich eslavich deleted the eslavich-fix-serialization-recursive-inverses branch April 24, 2020 13:18
pllim added a commit to pllim/astropy that referenced this pull request May 2, 2020
pllim added a commit to pllim/astropy that referenced this pull request May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement io.misc zzz 💤 io.misc.asdf archived: Go to asdf-astropy repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants