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

API spec-compliance fixes #145

Merged
merged 19 commits into from
Mar 8, 2023
Merged

Conversation

atolopko-czi
Copy link
Member

@atolopko-czi atolopko-czi commented Mar 2, 2023

@atolopko-czi atolopko-czi self-assigned this Mar 3, 2023
@atolopko-czi atolopko-czi marked this pull request as ready for review March 3, 2023 01:20
|----------------|-----------------------------------------------------------------------------------------------|
| `dense` | Return an iterator of `Arrow Tensor`s containing slice values. |
| `coos` | Return an iterator of `Arrow.SparseCOOTensor`s containing COO-encoded coordinates and values. |
| `table` | Return an iterator of `Arrow.Table`s containing COO-encoded coordinates and values. |
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if csr, csc, and record-batch should be removed from the spec, but as they're no longer in the tiledbsoma impl, I removed them for now. If intended to remain, perhaps we should indicate that implementations can optionally implement formats.

Copy link
Member

Choose a reason for hiding this comment

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

They need to be removed everywhere & it's a mistake on my part if I missed removing them from anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for confirming. Will stick with the change as is; no other occurrences exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, record-batch is still in the core code so reinstated here

@johnkerl johnkerl changed the title api spec compliance fixes API spec compliance fixes Mar 3, 2023
Copy link
Member

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

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

As above

@johnkerl johnkerl self-requested a review March 3, 2023 16:50
@atolopko-czi
Copy link
Member Author

As above

Sorry, I'm not following what change is being requested by this.

@bkmartinjr
Copy link
Member

changes to date seem good. I left a few suggestions in single-cell-data/TileDB-SOMA#834 about additional cleanup of the open issues or to be further specified notes.

The unresolved issues will be reincarnated as GitHub Issues.

In other words, every `SOMAMeasurement` has a distinct set of variables (features), and inherits common observables from its parent `SOMAExperiment`. The `obs` and `var` dataframes define the axis annotations, and their respective `soma_joinid` values are the indices for all matrixes stored in the `SOMAMeasurement`.

<figure>
<img src="images/SOMAExperiment.png" alt="SOMAExperiment">
</figure>

> ⚠️ **Issue**: it would be a good idea to factor `SOMAExperiment` and `SOMAMeasurement` into separate sections.
Copy link
Member Author

Choose a reason for hiding this comment

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

Now an issue: #147


1. Are there operations specific to SOMAExperiment and SOMAMeasurement that need to be defined? Or do they inherit only the ops from SOMACollection?
2. What (if any) additional semantics around writes need to be defined?
3. Value filter support in `NDArray` has been proposed:
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to #148

Issues to be resolved:

1. Are there operations specific to SOMAExperiment and SOMAMeasurement that need to be defined? Or do they inherit only the ops from SOMACollection?
2. What (if any) additional semantics around writes need to be defined?
Copy link
Member Author

@atolopko-czi atolopko-czi Mar 3, 2023

Choose a reason for hiding this comment

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

Deleting (2) for good. I believe write semantics have been well documented now in the Object Lifecycle section, w.r.t. atomicity and concurrency semantics.


Issues to be resolved:

1. Are there operations specific to SOMAExperiment and SOMAMeasurement that need to be defined? Or do they inherit only the ops from SOMACollection?
Copy link
Member Author

Choose a reason for hiding this comment

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

Bruce votes for deleting and so do I, so deleting (1) for good.

@@ -70,6 +70,7 @@ The foundational types are:
The composed types are:

- `SOMAExperiment`: a specialization and extension of `SOMACollection`, codifying a set of naming and indexing conventions to represent an annotated, 2-D matrix of observations across _multiple_ sets of variables.
- `SOMAMeasurement`: a specialization and extension of `SOMACollection`, that contains a set of annotated observables that are common to one or more sets of measurements and/or derived calculations.
Copy link
Member Author

Choose a reason for hiding this comment

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

For #133

@johnkerl johnkerl changed the title API spec compliance fixes API spec-compliance fixes Mar 3, 2023
@atolopko-czi
Copy link
Member Author

@thetorpedodog @mlin Let me know if you intend to review these spec changes, in which case I'll hold off on merging. Ty!

abstract_specification.md Outdated Show resolved Hide resolved
@atolopko-czi atolopko-czi merged commit f6c0faa into main Mar 8, 2023
@atolopko-czi atolopko-czi deleted the atol/834-api-spec-compliance-fixes branch March 8, 2023 14:53
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.

Update a Measurement section in the spec [python] Review TileDB-SOMA for spec compliance
4 participants