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

chunkedgraph module documentation improvements and standardization #129

Merged
merged 10 commits into from
Nov 24, 2023

Conversation

bdpedigo
Copy link
Collaborator

@bdpedigo bdpedigo commented Oct 25, 2023

Summary

  • Added documentation for some returns which have a dictionary with many fields or dataframe etc.
    • This was my main reason for this PR, as a new user I just found the meaning of many of these kinds of returns opaque.
    • This could probably still be improved. I took a stab at documenting these fields but if I could be wrong, feel free to add corrections.
  • Made everything use numpydoc, previously it was a mix
  • Used a more standard way of specifying types (see numpydoc)
  • Clarified references to actual variables in the docstrings with backticks
  • Added a few missing param descriptions
  • Tried to standardize nomenclature a bit, at least in this module

TODO

  • fix remaining inline TODOs about clarifying units
    • asked for feedback on a couple
  • build docs locally to test

Timestamp of the operation.
"user_id": int
User who performed the operation.
# TODO not actually sure what the rest of the outputs are, not an admin
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this command didn't work for me (even with a non-zero user_id, so an admin would need to fill this out.

perhaps also worth clarifying who can use this function at all?

Copy link
Collaborator

Choose a reason for hiding this comment

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

good point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bethany also had the same issue as me - I could open a new issue if this is meant to be available to everyone as the docs currently indicate, or if not, we could clarify here.

Although, if user info is not meant to be accessible to everyone, this info is available in the tabular changelog, so there's already a clunky workaround

Choose a reason for hiding this comment

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

current user of the clunky workaraound

Copy link
Collaborator

Choose a reason for hiding this comment

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

We were conservative in what we made available to users to easily follow other peoples work. I think a discussion can be had whether this should be behind admin privileges. For giving the necessary privileges: It is easy to add dataset-specific admins, so it would be straightforward to add the two of you as that (and we should do that). While the data is available in tabular changelogs, one would still have to iterate through all neurons to trace another users actions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's fair. for the purposes of this PR, I just added a note to the documentation indicating that this is only supported for admins.

separately, yes, I'd be happy to 1) get access but more importantly 2) talk about opening up this function. I don't care too much, but since there's already a roundabout way to get that info it seems like it would make sense to support for any user (or closing that loophole if not).

caveclient/chunkedgraph.py Outdated Show resolved Hide resolved
caveclient/chunkedgraph.py Outdated Show resolved Hide resolved
@bdpedigo
Copy link
Collaborator Author

I think I've addressed everything - but lmk if any further changes are requested

@fcollman fcollman merged commit 31e12e3 into CAVEconnectome:master Nov 24, 2023
1 check passed
@bdpedigo bdpedigo deleted the documentation-improvements branch November 24, 2023 22:44
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.

5 participants