-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat(core): add models and class for pycytominer core #383
base: main
Are you sure you want to change the base?
feat(core): add models and class for pycytominer core #383
Conversation
@d33bs @shntnu @gwaybio @bethac07 I wanted to bring this WIP PR to your attention. It represents the approach I'd like to take for the planned refactor/modularization of pycytominer. In essence I'd like to create an experimental Two python packages that will be at the "core" of One additional submodule I'm planning to add to Perhaps it would be worthwhile to get together sometime and whiteboard a UML class diagram and/or discuss this planned approach more generally. |
Thanks for your initiative on this @kenibrewer! It's great to see this evolution. Roadmap questionCan you point me to where this work fits in the CZI EOSS pycytominer roadmap? Would it be in "(4) Add data class for CytoTable input to enable flexible metadata and feature management"? Also, how would this work fit in "(2) Implement Python Fire for CLI optionality"? But maybe this is also separate pre-work that is required prior to hitting the deliverables? Development questionI'm also a bit weary of the development/release schedule. Given the answer to the previous questions, would the expectation of regular breaking changes negatively impact development for other pycytominer deliverables? I believe we discussed development in other branches, but that you (or maybe it was @d33bs ) were generally against developing on off-main branches? My goal is to identify a development/release solution that minimally impacts our users (maybe this is ok since we have conda/pypi) while enabling us to hit other pycytominer milestones without extensive blocking. Pycytominer chatI'm in favor of diagramming and more in depth discussion. Should anyone other than the folks you listed be on the call? Maybe @ErinWeisbart and @axiomcura ? I can facilitate this, and welcome any other ideas. Thanks again Ken! |
Thanks @kenibrewer for opening up the discussion here with some example work to match! Generally I feel that Pandera can be a powerful tool and might serve pycytominer well. At the same time, I wonder a bit about how data vagueness and inference are helpful to the work that takes place in pycytominer. I'd be 👍 for a meet to discuss this a bit further with others! Diagramming could be really helpful. Maybe a shared HackMD or Excalidraw board could assist (and also be included within the repo where helpful)? I'll try to be specific about items that I feel might be important to this discussion:
|
I'll respond in more detail later, but here are some brief responses. Stories@d33bs Thanks for the suggestion on the stories. I'll add those to the three linked issues on this PR. RoadmapThis new core module would be in support of nearly all planned milestones:
Has no impact on ability to deliver CLI interfaces. Addresses a substantial portion of the technical debt planned to be addressed in cyto_utils.
Core represents an alternative (and likely cleaner) approach to attaching a long list of decorators to the main functions (annotate, normalize, etc)
Core would greatly simplify testing
Core would enable this milestone too. Breaking ChangesAn important clarification on breaking changes. The plan for this approach would enable us to have no breaking changes anywhere in the existing codebase. Initially MeetingLet's schedule a meeting 2-3 weeks from now so I can prepare a more detailed plan and overview of my ideas. Great idea on including @ErinWeisbart and @axiomcura. |
Thanks @kenibrewer - I'll work on scheduling. I think ~1 month is good lead time. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
==========================================
- Coverage 94.96% 94.03% -0.93%
==========================================
Files 56 58 +2
Lines 3137 3168 +31
==========================================
Hits 2979 2979
- Misses 158 189 +31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Description
This represents initial work toward an experimental
core
module that is intended to the basis of refactoring pycytominer towards a more modular and object oriented design.What is the nature of your change?
Checklist
Please ensure that all boxes are checked before indicating that a pull request is ready for review.
📚 Documentation preview 📚: https://pycytominer--383.org.readthedocs.build/en/383/