Skip to content

Debugs for reading a .pickle file with signals#15

Open
CodyCBakerPhD wants to merge 6 commits intoleiferlab:masterfrom
catalystneuro:fix_setup
Open

Debugs for reading a .pickle file with signals#15
CodyCBakerPhD wants to merge 6 commits intoleiferlab:masterfrom
catalystneuro:fix_setup

Conversation

@CodyCBakerPhD
Copy link

@CodyCBakerPhD CodyCBakerPhD commented Apr 29, 2024

When trying to open some .pickle files for the NWB conversion project, I had to make some changes to this repo

Working on Windows, Python 3.12, but a lot of this would affect other platforms and even older Python versions

  • Remove legacy c++ from setup
  • add some core requirements like matplotlib to setup
  • fix deprecated numpy dtype usage
  • lazy imports for hard to install packages

I'd also recommend looking into getting this released on PyPI to simplify the installation process from requiring a git clone

@CodyCBakerPhD CodyCBakerPhD changed the title Remove broken legacy c++ stuff from setup Debugs for reading a .pickle file with signals May 6, 2024
@@ -1,27 +1,6 @@
#!/usr/bin/env python

from distutils.core import setup, Extension
Copy link
Author

Choose a reason for hiding this comment

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

Of course, this PR solves the problem by completely removing the C++ stuff

If you'd rather keep it, there are some alternative approaches I can investigate (like making it optional - or splintering it to a different repo) but they will take more time

import json
import pickle

import numpy as np
Copy link
Author

Choose a reason for hiding this comment

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

Moving this line was a minor style change - styling patterns such as isort recommend separating core Python libraries from third-party modules from the 'local' module

But if you prefer the first way just let me know; I didn't make this change holistically, but if you'd like that as well the easiest way would just be to setup pre-commit on the repo

Comment on lines -131 to +136
optogeneticsFrameCount = np.zeros(optogeneticsN, dtype=np.int)
optogeneticsNPulses = np.zeros(optogeneticsN, dtype=np.int)
optogeneticsRepRateDivider = np.zeros(optogeneticsN, dtype=np.int)
optogeneticsNTrains = np.zeros(optogeneticsN, dtype=np.int)
optogeneticsFrameCount = np.zeros(optogeneticsN, dtype=np.int64)
optogeneticsNPulses = np.zeros(optogeneticsN, dtype=np.int64)
optogeneticsRepRateDivider = np.zeros(optogeneticsN, dtype=np.int64)
optogeneticsNTrains = np.zeros(optogeneticsN, dtype=np.int64)
Copy link
Author

Choose a reason for hiding this comment

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

Numpy deprecated this behavior a while back (first a soft warning for several versions; then finally a hard removal that caused some of my errors)

There's actually 2 parts to it as well

i) np.int has become np.integer and is the 'base' data type of all specific np.int<number of bits> types
ii) they no longer allow dtype=np.integer which always defaulted a system specific np._int value - the modern recommendation is to set it to an explicit size

Comment on lines -15 to -17
import mistofrutta.struct.irrarray as irrarray
import wormdatamodel as wormdm
import savitzkygolay as sg
Copy link
Author

Choose a reason for hiding this comment

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

I moved each of these into the scopes of the functions that use them so someone can import the wormdatamodel package without needing to find each GitHub repo and do local installs

Using each function would still error if the package is missing, but at least that only occurs when (and 'if', in my case) those functions would be called

Copy link
Author

Choose a reason for hiding this comment

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

An alternative to this, if you prefer to keep things as they are, might be to include git-based installations in the setup requirements for this package

That could however cascade a series of PRs like this one if there's any trouble installing those packages however

@CodyCBakerPhD
Copy link
Author

@aleifer @emosb @SophieDvali

With the changes outlined on this PR I was able to successfully open a green.pickle file and read the signal!

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review May 6, 2024 19:04
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.

1 participant