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

RAD-120: Read Pattern #233

Merged
merged 6 commits into from
Apr 19, 2023
Merged

Conversation

PaulHuwe
Copy link
Collaborator

@PaulHuwe PaulHuwe commented Apr 14, 2023

Resolves RAD-120

Closes #

This PR adds read pattern to the exposure group.

Checklist

@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3d5b215) 93.54% compared to head (5b43f62) 93.54%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #233   +/-   ##
=======================================
  Coverage   93.54%   93.54%           
=======================================
  Files           4        4           
  Lines         124      124           
=======================================
  Hits          116      116           
  Misses          8        8           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@PaulHuwe PaulHuwe changed the title Fixed formatting. RAD-120: Read Pattern Apr 14, 2023
CHANGES.rst Outdated Show resolved Hide resolved
@PaulHuwe PaulHuwe marked this pull request as ready for review April 17, 2023 13:52
Copy link
Collaborator

@kdupriestsci kdupriestsci 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 to me. Filed related RDADS ticket RDADS-587

src/rad/resources/schemas/exposure-1.0.0.yaml Outdated Show resolved Hide resolved
@ddavis-stsci
Copy link
Collaborator

ddavis-stsci commented Apr 18, 2023

I'm a bit confused here.
Do we not need to preserve the grouping? For instance if the read pattern is
[1, 2], [3, 4, 5], [7, 8, 9], where 6 is skipped, turning this into an array yields,
array([1, 2, 3, 4, 5, 7, 8, 9])
but the read pattern [1, 2 ,3, 4, 5], [7, 8, 9] also gives the above array.
So how does the user find the grouping? Go back to the ma table names? or does this simply not matter here?

@schlafly
Copy link
Collaborator

I agree that the grouping is key here. But the type is an array of arrays, which looks fine to me?

@PaulHuwe
Copy link
Collaborator Author

I agree that the grouping is key here. But the type is an array of arrays, which looks fine to me?

It should be, but I can add an explicit test for this structural preservation in RCAL-543.

@ddavis-stsci
Copy link
Collaborator

ddavis-stsci commented Apr 18, 2023

I'm not sure that works for a list of list with an unequal number of elements, so if I try to convert the above list into an array I get

np.array([[1], [2, 3], [4], [5, 6, 7, 8]])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: setting an array element with a sequence. The requested array has an inhomogeneous shape after 1 dimensions. The detected shape was (4,) + inhomogeneous part.

Am I missing something?
I can turn that list into a 2-d array. Is that what we're proposing?
For the archive catalog that will be stored as the original list? e.g. '[1]', '[2,3]', '[4]', ...

@schlafly
Copy link
Collaborator

Is the RAD schema going to try to push that into a numpy ndarray as you're showing? Can we mark the type as list rather than array? We were imagining the archive storing str([[1], [2, 3], [4], [5, 6, 7, 8]]) = '[[1], [2, 3], [4], [5, 6, 7, 8]]'

@WilliamJamieson
Copy link
Contributor

This cannot be an "array of arrays", that would by definition require all of the member arrays to be the same shape.

In order for this to work it must remain a "list of lists" as each constituent list can have different shape.

If we must have an array compatible format then it would be better to build a 1D array whose index represents the reads and values represent the grouping. A null flag such as -1 can be used to for skipped reads. E.G.

[1, 2], [3, 4, 5], [7, 8, 9]

Would be instead represented as

[0, 0, 1, 1, 1, -1, 2, 2, 2]

@PaulHuwe
Copy link
Collaborator Author

@WilliamJamieson
Copy link
Contributor

Note that we can do this as unsigned integers by defining 0 as the skipped positions. Then the example becomes

[1, 1, 2, 2, 2, 0, 3, 3, 3]

description: |
Enumeration of detector reads to resultants making up the L1 data downlinked
from the observatory
type: array
Copy link
Contributor

Choose a reason for hiding this comment

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

The confusion may also be coming from this term. Recall, the schemas are written using "jsonschema", which is based on "json" originating in JavaScript. So the "type" refers to the type you would see in JavaScript, which happens to term "array" to mean a resizable indexed collection of items which in Python we would call a "list". Hence, the schema here is describing a "list of lists" in Python.

Copy link
Collaborator

@ddavis-stsci ddavis-stsci 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.

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.

6 participants