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

docs: add README for Curation API notebooks #620

Closed
wants to merge 7 commits into from

Conversation

danieljhegeman
Copy link

Initial README for now. Can be improved on in the future. Please comment if some aspect is glaringly wrong or a substantial omission has been made.

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #620 (1ebc9a5) into main (9bfa58c) will not change coverage.
Report is 3 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #620   +/-   ##
=======================================
  Coverage   83.00%   83.00%           
=======================================
  Files          19       19           
  Lines        1700     1700           
=======================================
  Hits         1411     1411           
  Misses        289      289           
Flag Coverage Δ
unittests 83.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -0,0 +1,25 @@
# Curation API notebooks
Copy link
Contributor

Choose a reason for hiding this comment

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

change to discovery and say formerly know as curation notebooks through out the doc

@brianraymor
Copy link
Contributor

I would prefer that this README is not added at this time. The curators are already quite familiar with the API. I'm planning to ask Clever Canary to perform some work in this area later.

@danieljhegeman
Copy link
Author

I would prefer that this README is not added at this time. The curators are already quite familiar with the API. I'm planning to ask Clever Canary to perform some work in this area later.

@brianraymor we got questions recently from another user who was new to the notebooks, and this question has come up from time to time from others in DM as well, including CC. I am not seeing a downside to adding this very simple README which can be deleted if necessary in the future (when CC improve upon it?).

@Bento007
Copy link
Contributor

Bento007 commented Sep 18, 2023

I would appreciate having this readme added. It avoids me a few extra click to figure out which flavor of discovery API I actually need to jump into. Could we ask Clever Canary to update or remove as needed, if they plan on doing something better?

### Python notebooks: `python/`

These notebooks are the most user-friendly; start here if you're unsure. All access token generation, url construction, and response
handling is abstracted away from the user. Unitary interactions such as getting a Dataset, creating a Collection, etc.,
Copy link
Contributor

Choose a reason for hiding this comment

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

creating a collection is a curator action, not a consumer action.

Copy link
Author

Choose a reason for hiding this comment

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

Correct. Do you mean to say that, as such, it should not be used as an example? Only consumer actions should be referenced here?


These notebooks show the construction of http requests at a more granular level, and as such may be useful for technical
users.

Copy link
Contributor

Choose a reason for hiding this comment

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

and as such may be useful for technical users.

This might be useful for someone who wants to port the REST API to another language or wants more control of the responses?

Copy link
Author

Choose a reason for hiding this comment

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

or wants more control over the request, yes (for us, at the moment, and partners, in the case of hitting a Discover API on rdev...)

Copy link
Author

Choose a reason for hiding this comment

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

changed to only reference consumer actions...


### R notebooks: `R/`

These notebooks are the same as the "raw" Python notebooks mentioned immediately previous, but in R.
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this directory should be named R_raw to be symmetrical?

Copy link
Author

Choose a reason for hiding this comment

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

Renamed (see diff below)

Copy link
Author

Choose a reason for hiding this comment

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

I can also move the entire notebooks/curation_api/ dir to notebooks/discover_api/ in a follow-up PR...

@danieljhegeman
Copy link
Author

@brianraymor if you are comfortable giving your ✅ then we can merge this

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

This PR has not seen any activity in the past 2 weeks; if no one comments or reviews it in the next 3 days, this PR will be closed.

@github-actions github-actions bot added the Stale label Oct 6, 2023
@danieljhegeman
Copy link
Author

@brianraymor ping -- hoping to merge this

Copy link
Contributor

@brianraymor brianraymor left a comment

Choose a reason for hiding this comment

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

I would also recommend replacing the references to individual notebooks in https://api.cellxgene.cziscience.com/curation/ui/#/ with something like:

See the [README](<insert link>] for example notebooks in python and R.


### Python notebooks: `python/`

These notebooks are the most user-friendly; start here if you're unsure. All access token generation, url construction, and response
Copy link
Contributor

Choose a reason for hiding this comment

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

access token generation is confusing for non-curation use which does not require access tokens?

@github-actions github-actions bot removed the Stale label Oct 7, 2023
@github-actions
Copy link
Contributor

This PR has not seen any activity in the past 2 weeks; if no one comments or reviews it in the next 3 days, this PR will be closed.

@github-actions github-actions bot added the Stale label Oct 22, 2023
@github-actions
Copy link
Contributor

This PR was closed because it has been inactive for 17 days, 3 days since being marked as stale. Please re-open if you still need this to be addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants