Functional model schemas #225
Conversation
cshanahan1
force-pushed
the
functional_model_schemas
branch
2 times, most recently
from
f25d7ec
to
7fc0b39
Compare
There was a problem hiding this comment.
We should add examples to these new schemas as well -- the asdf pytest plugin uses them to construct test instances and asserts that they validate under the schema and round-trip successfully.
Can you also add a version_map-1.5.0.yaml? It should be a copy of version_map-1.4.0.yaml with your new schemas added. One of the tests enforces that the list be in alphabetical order, so watch out for that.
| - $ref: "../unit/quantity-1.1.0" | ||
| - type: number | ||
| description: Effective (half-light) radius. | ||
| n: |
There was a problem hiding this comment.
@nden @perrygreenfield do you have an opinion on matching the modeling package parameter names vs using a name that is more descriptive? If someone were to inspect an asdf file written under this schema, they'd just see n and the schema description wouldn't be readily available. We could name it sersic_index, for example.
There was a problem hiding this comment.
I think it would be good practice for the ASDF representation to be more descriptive. Nothing requires it to use the same name that the modeling package uses.
There was a problem hiding this comment.
in this particular case i personally think the parameter n is widely understood to be the sersic index, but there are certainly other cases where this issue would apply
There was a problem hiding this comment.
I think names which are widely accepted should be used both in modeling and asdf.
For models which have definitions in the VO standard I think we should try to use the same names. I was going to suggest checking all names against the VO definitions when moving the transforms out of the standard.
|
yep, I'll add tests and make a new version map. speaking of - i'm trying to figure out why the tests are failing for #224 and I thought it was because the new schemas weren't in the map so I added them but the tests are still failing. even though that can be closed once this PR is good to go, i'd like to get those tests passing there now so I know exactly what I need to do for this one. |
This turned out to be an issue with the schema testing code -- it couldn't handle schemas whose Type classes don't exist yet. I opened a PR (asdf-format/asdf#771) to fix the problem. |
cshanahan1
force-pushed
the
functional_model_schemas
branch
from
70188fa
to
f8a45d8
Compare
cshanahan1
force-pushed
the
functional_model_schemas
branch
from
59890a4
to
8203e18
Compare
| - | | ||
| !transform/box1d-1.0.0 | ||
| amplitude: 10.0 | ||
| bounding_box: [-0.5, 3.5] |
There was a problem hiding this comment.
Since bounding_box isn't part of transform-1.2.0, should we wait until transform-1.3.0 to include it in the examples?
Or, maybe we should go ahead and create transform-1.3.0 now?
There was a problem hiding this comment.
should I go ahead make 1.3.0 and then have just the new models use this version? also, why is 1.1.0 the exact same as 1.0.0 (similarly, when does it make sense to make a new version of transform?)
There was a problem hiding this comment.
@nden what do you think? I'm inclined to say yes, just so we don't have to release the standard again right away.
I don't know why 1.1.0 was created with no changes -- it could be that at the time we assumed that the schema versions had to stay identical to the standard version.
| - type: object | ||
| properties: | ||
| amplitude: | ||
| anyOf: |
There was a problem hiding this comment.
This is tangential to this PR, but at some point we should consider creating a schema for a quantity-or-number type, since that gets used all over the place.
|
This looks good! One thing I noticed is a @eslavich Does it make sense to add a check in the tester that a version of every schema is included in versionmap files? |
There is a check, here, but I removed schema fragments (such as transform) from the version map in my refactor, since they don't have a |
|
In my latest commit I added some new non-functional-model schemas in here too. @nden by |
|
@shannnnnyyy Ed clarified he deliberately removed schema fragments, like I am not sure what the best way to deal with |
|
oh sorry i missed that comment! i'll leave the 1.3.0 thing alone then, and just keep adding new schemas here as I write them. |
Added schemas for all of the functional models except the few which are already done under polynomial (linear, multiply, shift, scale). If this is merged, #224 can be closed because this also includes the gaussians.