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

Initial Release for element-moseq #2

Merged
merged 93 commits into from
Mar 20, 2024

Conversation

MilagrosMarin
Copy link
Contributor

This PR introduces the initial release for element-moseq version 0.1.0 to the DataJoint repository, adhering to the standardized practices across DataJoint Elements. Here's a summary of the changes:

Changelog and version:

  • Added a CHANGELOG and version to document the changes made for this first release.

DevContainer Configuration:

  • Added and tested DevContainer configuration for GitHub Codespaces.
  • Mounted example_data from a deeplabcut project in the S3 bucket and validated in Codespaces.
  • Incorporated setup.py with extras_require and tests functionalities.

GitHub Actions:

  • Added GitHub Actions calling reusable workflows with .githubdirectory

Documentation:

  • Integrated documentation in docs regarding element-moseq and Keypoint-MoSeq.
  • Added README to align with DataJoint Elements standards.

Pipeline Development:

  • Developed kpms_reader readers.
  • Designed and executed the element_moseq pipeline architecture containing kpms_pca and kpms_model modules.

Images:

  • Created flowchart and pipeline images in the images directory

Notebooks:

  • Created a tutorial.ipynb notebook consistent with other DataJoint Elements.
  • Validated tutorial imports in Codespaces.
  • Introduced tutorial_pipeline.py to facilitate schema importation and activation.

Other files:

  • Included spelling, markdown, and pre-commit configuration files.
  • Added LICENSE, CONTRIBUTING, CODE_OF_CONDUCT files.

Code formatting:

  • Applied Black formatting

This PR marks a significant milestone in the advancement of element-moseq, laying the groundwork for future enhancements and contributions.

@MilagrosMarin MilagrosMarin mentioned this pull request Mar 20, 2024
5 tasks
@MilagrosMarin MilagrosMarin requested review from dimitri-yatsenko and ttngu207 and removed request for dimitri-yatsenko and ttngu207 March 20, 2024 09:18
Copy link
Contributor

@ttngu207 ttngu207 left a comment

Choose a reason for hiding this comment

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

@MilagrosMarin great work!
The design and implementation are well structured and cleanly executed.
I left a few comments.

For next steps, we will want to do some further cleanup, no major changes in the logic

  1. rethink the naming
    • kpms_pca -> moseq_train
    • kpms_model -> moseq_inference
  2. activate - we don't need to activate the 2 modules twice, as they go hand in hand (users can't use one without the other). We can use the same implementation as in element-calcium-imaging for scan and imaging modules

element_moseq/kpms_pca.py Show resolved Hide resolved
element_moseq/kpms_pca.py Show resolved Hide resolved
element_moseq/kpms_pca.py Show resolved Hide resolved
element_moseq/kpms_pca.py Show resolved Hide resolved
element_moseq/kpms_model.py Show resolved Hide resolved
element_moseq/kpms_model.py Show resolved Hide resolved
Copy link
Collaborator

@kushalbakshi kushalbakshi left a comment

Choose a reason for hiding this comment

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

Looks great overall, @MilagrosMarin. Just a few changes in the docs section. I haven't looked through the tutorial yet but can do so after we've finalized all the table names and the logic in the make functions in these rounds of review.

.github/workflows/release.yaml Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
docs/src/index.md Show resolved Hide resolved
docs/src/partnerships.md Show resolved Hide resolved

| Table | Description |
| --- | --- |
| Device | Scanner metadata |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this Element use the Device table? Or are we too far downstream to need this here?

docs/src/pipeline.md Show resolved Hide resolved
docs/src/pipeline.md Show resolved Hide resolved
element_moseq/kpms_pca.py Show resolved Hide resolved
element_moseq/kpms_pca.py Show resolved Hide resolved
@ttngu207
Copy link
Contributor

Merging this first PR - new revisions will be in separate PRs

@ttngu207 ttngu207 merged commit 1dea4e4 into datajoint:main Mar 20, 2024
Copy link
Contributor

@ttngu207 ttngu207 left a comment

Choose a reason for hiding this comment

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

@MilagrosMarin I've add further review comments on this PR - please incorporate the changes to the new PR as well

element_moseq/kpms_model.py Show resolved Hide resolved
element_moseq/kpms_model.py Show resolved Hide resolved
element_moseq/kpms_model.py Show resolved Hide resolved
element_moseq/kpms_model.py Show resolved Hide resolved
element_moseq/kpms_model.py Show resolved Hide resolved
element_moseq/kpms_model.py Show resolved Hide resolved
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.

3 participants