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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work-in-Progress: Modularizing nltools #412

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Work-in-Progress: Modularizing nltools #412

wants to merge 2 commits into from

Conversation

ejolly
Copy link
Collaborator

@ejolly ejolly commented Jan 19, 2023

Goal

The goal of this PR is prep a new version of nltools that uses our new package burnt-ends (currently private during development) as a core dependency for common statistics and utility operations.

Why?

It allows up to "lighten up" the core codebase of nltools making it more maintainable in the long run. Lower maintenance burden -> more fun 馃槑 .

It also allows us to modularize functionality that we have recapitulated across several different Python libraries written by lab members. E.g. both pymer and nltools rewrite essentially the same numpy-base linear regression routines and could instead depend on a single well-tested library to implement this core functionality (burnt-ends). Then each library can simply right the "wrapping code" around this functionality in API-compatible way without having to sweat the finer details.

There's probably a formal development term for this, but it's in essence the loose idea of of "functional core, imperative shell" but across multiple toolboxes rather than within a single toolbox.

When

We hope to have this implemented along with a documentation site for burnt-ends rather quickly. We're currently targeting a 0.5.0 release given the substantial installation/dependency change and secondary changes this PR will likely create.

Anticipated and unexpected slow downs will be logged in this PR. Currently our only anticipated slow-down is in existing code-areas that lack enough testing coverage for the current functionality of nltools. In these cases, we will need to generate new test code for burnt-ends rather that simply porting over existing testing code.

Proposed Redesign

After pip install burnt-ends, we intend for functionality to be ported over in a fashion like this:

# inside nltools/stats.py

__all__ = [...'regress']

# Seamlessly aliases to nltools.stats.regress for importing
from ends.stats import regress
# Inside ends/stats.py

def regress(...):
   # general purpose regress function
   ....  

Milestones

  • Deploy release 0.1.0 of burnt-ends to pypi with extractednltools functionality
  • Have nltools GA test-suite install burnt-ends from pypi and run (with possible failures)
  • Full nltools GA test suite passes with new burnt-ends dependency structure
  • Deploy release 0.2.0 of burnt-ends to pypi with extracted pymer4 functionality
  • Deploy release 0.3.0 of burnt-ends to pypi with extracted neighbors functionality

@ljchang Let me know if I'm missing anything

@ejolly ejolly marked this pull request as draft January 19, 2023 02:59
@ljchang
Copy link
Member

ljchang commented Feb 15, 2023

this sounds great to me. I added a bunch of stats modules from nltools and their tests, but haven't updated them to all work yet with dependencies. I also started organizing burnt-ends into modules.

I thought it would be good to pull Adjacency class in as it is not a brain specific stats module. At some point, we may want to rename it and potentially subclass it for Adjacency, similarity, distance, transition probability, and directed graphs. Right now it is super easy to use for all of those, and you specify the type using "matrix_type", but it is not super comprehensive. I'm not sure if it will be easier for users vs developers to substantially refactor into subclasses or keep a more general one that works for all things as it currently stands.

I also think we should start a stats_output class to standarize the output of all of our inference functions, e.g., regress, permutation, etc.

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.

None yet

2 participants