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

Update aeon_mecha using the new swc.aeon package #461

Open
wants to merge 7 commits into
base: datajoint_pipeline
Choose a base branch
from

Conversation

ttngu207
Copy link
Contributor

@ttngu207 ttngu207 commented Feb 5, 2025

  • remove obsolete aeon packages/modules
    • analysis
    • io
    • schema
  • remove aeon.io tests
  • import from swc.aeon instead
  • update dj_pipeline ingestions to using swc.aeon package
  • tested with a few representative ingestions: SLEAP data, Environment data (BlockStates, SubjectState, etc.),

What's next: update test suite for dj_pipeline (probably a separate PR)

Copy link
Contributor

@lochhh lochhh left a comment

Choose a reason for hiding this comment

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

Thanks @ttngu207 ! Only a few minor comments and we can discuss introducing the swc namespace in a separate issue/PR.

@@ -24,18 +24,14 @@ dependencies = [
"bottleneck>=1.2.1,<2",
"datajoint>=0.13.7",
"datajoint-utilities @ git+https://github.com/datajoint-company/datajoint-utilities",
"dotmap",
"swc-aeon @ git+https://github.com/SainsburyWellcomeCentre/aeon_api.git",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"swc-aeon @ git+https://github.com/SainsburyWellcomeCentre/aeon_api.git",
"swc-aeon",

use pypi swc-aeon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure the overall status of swc-aeon package, hence the install from github. We could easily switch to pypi

Comment on lines +8 to +9
import swc.aeon.io.reader as io_reader
from swc.aeon.io import api as io_api
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import swc.aeon.io.reader as io_reader
from swc.aeon.io import api as io_api
from swc.aeon.io import api as io_api
from swc.aeon.io import reader as io_reader

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but somehow, ruff put those in that order

@@ -8,9 +8,11 @@
import datajoint as dj
import pandas as pd

import swc
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import swc
import swc
from swc.aeon.io import api as io_api
from swc.aeon.io import reader as io_reader

Comment on lines +14 to +15
from swc.aeon.io import api as io_api
from swc.aeon.io import reader as io_reader
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from swc.aeon.io import api as io_api
from swc.aeon.io import reader as io_reader

Comment on lines +118 to +120
reader = {"swc": swc, "aeon": aeon}
for idx, n in enumerate(stream_detail["stream_reader"].split(".")):
reader = reader[n] if idx == 0 else getattr(reader, n)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still have any readers in aeon? could we just have reader = swc.aeon and getattr(reader, module path after "aeon." )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do, in aeon we still may create new Stream with new Reader based on the existing readers in swc

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.

2 participants