Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
extract1d for IFU data version 2- JP 1736 #5506
extract1d for IFU data version 2- JP 1736 #5506
Changes from 58 commits
09f3f79
3f125dc
5ed9130
3db8ead
44057bc
a206a6c
1440140
b7064d5
0cc84aa
1bc715a
5eecb27
d9019f5
290c14e
49a2540
aaf1e28
29a5504
50d4948
7336a0e
7daf396
bc3b291
473a65b
107eb93
8730676
054a850
fa30cb4
f611712
c4e82b2
dffe53d
066ca3f
182723f
0cc7632
1230785
968425a
fbb73bd
f6222b1
0a3caff
ca31cbc
27fae8d
02f3771
898ee06
33e8789
3d072db
e519b9a
ed406e0
058757a
8440768
e35d248
8448d2f
41af7e5
da95409
3cea987
3d48ba3
7edf50c
09fe97a
e62a5d6
30c62db
b04466b
06e800f
b3bcc1c
196b89a
5712155
c55ca7c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it would throw a big wrench into things at this late stage to rename this new model class to
Extract1dIFUModel
just for consistency in naming with the existingExtract1dImageModel
? If so, it's not a big deal.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is fine I can change the names of models to be more consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think that means I'll have to change some things about my generation code as well. Are the 'datamodl' keywords still meant to be MirMrsExtract1dModel and MirMrsApcorrModel ? If so I should only need to change the names of the schema files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @drlaw1558 that does imply that the actual class name of the data model used for the extract1d reference file will be changing from
IFUExtract1dModel
toExtract1dIFUModel
. That's only for the extract1d reference file model. The apcorr ref files models remain unchanged. I don't seeMirMrsExtract1dModel
referenced anywhere, or am I just missing it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it looks like this was the name Jane used in creating a test asdf file back in October; maybe that's now been superseded by MirMrsExtract1dModel -> IFUExtract1dModel -> Extract1dIFUModel. In this case I'll need to change both my code and the reference file to make sure the data model keyword is up to date.
It doesn't look like the update has been made in the schema for this branch yet though? @jemorrison your comment above said that it had been changed, but the relevant branch still seems to have the old names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed over the new code. It is Extract1DIFUModel now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see it. If we're now using schemas/extract1difu.schema.yaml should we remove the old schemas/mirmrs_extract1d.schema.yaml ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again in terms of file naming consistency, could this new schema file be renamed to
extract1difu.schema.yaml
, in keeping with the existingextract1dimage.schema.yaml
file?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are some data type attributes named
type
while others aredatatype
? Does it make a difference? My inner hob goblin of foolish consistency suggests having them all the same.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was trial and error to get this to work. This worked that is all I know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So from trial and error I have found that when I create the asdf reference files, write them to disk and then read them in
using asdf.open and test them using asdf validate - all values that are strings that are called datatype in the schema fail in the validate. The error message says they are not arrays. So I am guessing that 'datatype' is for arrays and 'type' is for single values.