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

Integrate your bugfixes with nauralcodinglab/openscope_surround #6

Open
wants to merge 76 commits into
base: master
Choose a base branch
from

Conversation

efharkin
Copy link
Contributor

Hi Saskia, please consider merging this PR to bring your code up to date with what Kai Lun, Remi, and I are using. There are quite a few commits because this includes all of the work from Kai Lun and I since July 2020, but nothing before May 19, 2021 overlaps with your work. The commits from May 19 onwards clean up the repo but don't make any changes to how your code works, with three exceptions:

  • I fixed a couple of scripts that were broken in your version due to merge conflicts (marked with <<<<<<<</>>>>>>>) by restoring your work.
  • I ported a few scripts to python 3 using 2to3 which is an official python tool for migrating code; this mainly added brackets to print statements, but you can review the exact changes in commits that mention python in the commit message (eg 59b2ddf). Apparently Kai Lun had been running everything in python 3 already, so the affected code might be dead anyway.
  • I moved files that were mainly meant to be imported into the oscopetools package; this won't affect you unless you don't have oscopetools installed. Run pip install -e path/to/openscope_surround (preceded by conda activate your_env_name if you use Anaconda, as described in the README) if you don't have it.

How to merge

  1. Make a backup copy of your working copy of openscope_surround by right-clicking the openscope_surround folder and choosing "Compress openscope_surround"
  2. Open a terminal and move to your openscope_surround folder by typing cd and then dragging the folder icon at the top of your Finder window into the terminal
  3. Run the following command
git remote add efharkin https://github.com/efharkin/openscope_surround.git &&
git fetch efharkin &&
git checkout -b before_merge &&
git add -A &&
git commit -m "Current state of Saskia's working directory" &&
git diff --quiet HEAD b641ee640e08c034b77d842094d29b473301f5cf &&
echo "Working directory is identical to Emerson's copy. Ok to proceed with merge." ||
echo "Working directory has changed, don't proceed with merge. Send backup to Emerson and restore your copy from backup."
  1. If the output is "Ok to proceed with merge", please run the following command to merge it into your working copy (the PR will be accepted automatically the next time you push)
git checkout HEAD^ &&
git merge --ff-only efharkin/cleanup &&
echo "Success!" ||
echo "Unexpected error. Please copy contents of terminal into TextEdit and send to Emerson."
  1. git push
  2. You can replace the before_merge branch with a tag (git tag -a before_merge before_merge && git branch -D before_merge) or just delete it.

Details about commits that touch your code

The commits from May 2021 are dedicated to merging scripts that were duplicated across analysis and oscopetools. I made sure that no work would be lost during the merge by carefully reviewing differences using git diff --no-index oscopetools/x.py analysis/x.py for each script. This was actually a pretty easy process because the files were mostly exact copies or close to it. I documented what I did in the commit messages, which you can read by clicking on them below, and in my PR on the nauralcodinglab page.

The commits from June 2021 are dedicated to merging work that you sent me over Slack on June 3. Basically I committed everything you sent me, cleaned it up a bit, and then merged the nauralcodinglab version. The cleanup basically consisted of removing more duplicated material and applying autoformatting. As with the May commits, I was careful not to lose any of your work.

efharkin added 30 commits July 1, 2020 19:36
Make read_data.{get_dff_traces, get_raw_traces} build RawFluorescence
objects instead of returning numpy arrays.

RawFluorescence is untested.
- Break up oscopetools.read_data into multiple files. Changes nothing
  for clients--all the functions and classes are still loaded into the
  `read_data` namespace.
- Introduce a `CenterSurroundStimulus` object to represent a
  center-surround stimulus and make
  `get_stimulus_table('path/to/dfile.h5', 'center_surround')` load it.
Run `make doc` to generate HTML documentation for oscopetools.
Optionally copy fluorescence datasets more quickly by getting a
read-only view of the underlying fluorescence array instead of copying
it. This provides an order of magnitude speedup of slicing operations
such as `get_time_range`.
- Autoformat `dataset_objects.py`
- Add `trial_mean()` and `trial_std()` to `TrialFluorescence`
This patch allows Orientation to be initialized with None/np.nan, but
iterating over Orientation no longer produces Orientation(None).
- Allow orientations to be added to or subtracted from eachother or
  numeric types. The result is guaranteed to be a valid Orientation.
Remove a guard against trying to get a negative time window in
TimeseriesDataset. In some cases, t=0 might be the start of a
trial, so t<0 might represent a valid baseline period.
- Include a `data` attribute in implemented `Datasets` to provide access
  to underlying data.
- Replaces the `fluo` attribute on `Fluorescence` classes and the
  `_dframe` attribute on `Eyetracking`
Getting a single cell or a single trial from a `Fluorescence` or
`TrialDataset` yields an array with one or more dimensions of size one.
For example,

>>> fluorescence.get_cells(0).data.shape
[10, 1, 462]

The resulting array can usually be expressed as a matrix, which is
convenient for passing to matplotlib plotting functions. Before this
patch, this was done by manual slicing to remove dimensions of length
one.

>>> plt.plot(fluorescence.get_cells(0).data[:, 0, :].T)

This patch switches to using `np.squeeze()`, which produces the same
result without having to remember the order of the dimensions (except
that time is last).

>>> plt.plot(fluorescence.get_cells(0).data.squeeze().T)
kt1524 and others added 30 commits July 30, 2020 20:09
Replaced is_trial with issubclass().
Added greedy pixelwise RF and overlapping index
This reconciles differences between saskiad/master and
nauralcodinglab/master. There are no direct conflicts, and the merge
consists almost entirely of adding files from saskiad/master that were
missing from nauralcodinglab/master, and vice-versa.

The only potentially conflicting change is that setup.py now raises an
error if a user tries to install oscopetools using Python 2.

Note that scripts from saskiad/master:analysis exclusively use the
version of the read_data.py module from saskiad/master:analysis. There's
also a read_data.py in oscopetools, which has changed substantially
since nauralcodinglab/master and saskiad/master diverged (mainly
refactoring, but other changes are possible). This could result in
scripts in analysis behaving differently if the version of read_data
being imported is changed in the future.
Corrected overlapping index equation
Several commits are squashed because the changes from the "Added gag and
save_data" commits are already in 987b17e in nauralcodinglab/master, and
674bbcb reverses 953bdba.  The remaining commits resolve trivial
conflicts between kt1524/master and nauralcodinglab/master.

Squashed commit of the following:

commit adc23dfa6fa6a7f24314b15b5d4f1e45908a0015
Author: Kai Lun <[email protected]>
Date:   Wed May 19 01:11:01 2021 +0200

    Resolve conflicts

commit 62b15a18e2bb4c07b7add9f934f5b76ae60ae81b
Merge: 674bbcb 987b17e
Author: Kai Lun <[email protected]>
Date:   Wed May 19 00:36:42 2021 +0200

    resolve conflicts

commit 674bbcb32811f93caa26ab1f4e028af44674e53b
Author: Kai Lun <[email protected]>
Date:   Tue May 18 23:29:19 2021 +0200

    Remove path appending

commit 953bdba87e1af62c45239d8adfc4cdec3daabdaf
Author: Kai Lun <[email protected]>
Date:   Mon May 17 17:29:54 2021 +0200

    Appended oscopetools directory to sys

commit 58c006e6b19f3ce90c1b73bb05b03dd117ab8143
Author: Kai Lun <[email protected]>
Date:   Wed May 5 01:37:09 2021 +0200

    Added gag and save_data

commit b66c92fa3f7d7e6c6f9f7da014da521b2aef8c83
Author: Kai Lun <[email protected]>
Date:   Wed May 5 01:32:24 2021 +0200

    Added gag and save_data

commit c12e3c5f233ee7df7c4db24ad095c73305255403
Author: Kai Lun <[email protected]>
Date:   Wed May 5 01:30:58 2021 +0200

    Added gag and save_data
Add shell script for autoformatting Python files with consistent
settings.
At the time of this commit, there are two important differences between
analysis/stim_table.py from Saskia's current master (9e5c7b4) and
oscopetools/stim_table.py from nauralcodinglab/master.py (5faf45d):

1. analysis/stim_table.py stores the center coordinates of the stimulus,
   but oscopetools/stim_table.py does not.
2. analysis/stim_table.py:get_attribute_by_sweep() contains a try/except
   block to handle attribute_by_sweep being a list rather than a scalar.

(The rest of the differences between oscopetools/stim_table.py and
analysis/stim_table.py are due to formatting or Python 2 syntax in
analysis/stim_table.py.)

This commit incorporates these two changes from Saskia into
oscopetools/stim_table.py. Anyone with the oscopetools package installed
can now use analysis/stim_table.py by adding `from oscopetools import
stim_table` to their scripts.
analysis/RunningData.py and oscopetools/RunningData.py are functionally
identical as of 5d124ef. The only
difference between them is the syntax used to import
oscopetools/stim_table.py:

from oscopetools.stim_table import xyz  # analysis/RunningData.py
from .stim_table import xyz  # oscopetools/RunningData.py

This commit removes the copy from the analysis directory and adjusts
imports accordingly.
analysis/get_eye_tracking.py and oscopetools/get_eye_tracking.py are
completely identical as of 0520bf2.
Remove the copy from analysis and adjust imports accordingly.
analysis/get_all_data.py:get_all_data() contains commented-out code that
is virtually identical to
oscopetools/get_eye_tracking.py:align_eye_tracking(). The only
differences are that the `.values` attributes are missing in the
following lines of commented-out code:

pupil_area = pd.read_hdf(dlc_file, 'raw_pupil_areas').values
eye_area = pd.read_hdf(dlc_file, 'raw_eye_areas').values

Since this code already exists in oscopetools/get_eye_tracking.py, I
don't see a reason to keep the commented out copy. This commit removes
the commented-out code and makes no other changes.
Autoformatting using Black. See autoformat.sh in the project root for
autoformatting settings. Changes are mainly wrapping long lines and
making consistent spacing around function arguments and operators.

This commit does not change the function of analysis/get_all_data.py in
any way.
oscopetools/get_all_data.py will be superseded by
analysis/get_all_data.py, but we need to make sure
oscopetools/get_all_data.py doesn't have any important changes before it
is removed (ideally, in the next commit). This commit incorporates three
features of analysis/get_all_data.py into oscopetools/get_all_data.py so
that diffs of these two files will be easier to read.

1. Remove code to load ellipse.h5, which is never used.
2. Remove code for aligning eye tracking and use the basically-identical
   align_eye_tracking() instead.
3. Change `for key in list(stim_table.keys())` -> `for key in
   stim_table.keys()`
oscopetools/get_all_data.py is out of date compared with
analysis/get_all_data.py. I'm removing oscopetools/get_all_data.py and
leaving the version in analysis because get_all_data.py will probably
only ever be used as a script, so it doesn't make sense to include in
the oscopetools package.

Summary of differences between analysis and oscopetools versions
----------------------------------------------------------------

The version of get_all_data.py in oscopetools is missing code to drop
ROIs with NaN values in the fluorescence trace. The two versions are
otherwise almost identical apart from the code behind the `if __name__
== '__main__'` guard. This part of analysis/get_all_data.py actually
contains all of the corresponding code from oscopetools/get_all_data.py
in comments, so there's no reason to keep oscopetools/get_all_data.py
around.
This commit captures the state of Saskia's working directory as sent to
me over Slack on June 3, 2021.

Note that the following files are broken due to unresolved merge
conflicts:

analysis/example_code/locally_sparse_noise_events.py
oscopetools/locally_sparse_noise.py

The following files are broken due to Python 2 print statements:

analysis/stim_table.py
analysis/locally_sparse_noise.py
analysis/DGgrid_analysis_5x5_nikon_SdV.py
oscopetools/locally_sparse_noise.py
- Remove analysis/sync since it's identical to oscopetools/sync and edit
  imports in analysis/stim_table.py and
  analysis/DGgrid_analysis_5x5_nikon_SdV.py accordingly.
- Restore executable permissions on oscopetools/sync/gui/__init__.py.
  Not sure whether executable permissions were removed accidentally, nor
  why this file was executable in the first place.
- Remove analysis/.ipynb_checkpoints, which shouldn't be tracked.
  (.ipynb_checkpoints is in .gitignore on another branch.)
- Remove analysis/test.py, which just prints the contents of some
  directories and probably shouldn't be tracked.
Remove spaces at the end of each line and on blank lines. No other
changes are made.
This fixes the files that were broken by merge of pull request saskiad#5
from efharkin/master into saskiad/master (ad507cd):

analysis/example_code/locally_sparse_noise_events.py
oscopetools/locally_sparse_noise.py

Files were fixes by choosing Saskia's version of
analysis/example_code/locally_sparse_noise_events.py and moving
analysis/locally_sparse_noise.py into oscopetools, since the analysis
version contains changes from Saskia.

These files were broken by the merge because uncommitted changes in
Saskia's working directory were auto-stashed (stash 37aec39 on top of
commit 1ae1800) before the merge and couldn't be re-applied cleanly.
This happened because my branch (ending in c6d2628) auto-formatted some
blocks that Saskia had been working on. This commit keeps Saskia's
changes without applying auto-formatting (that will happen in a future
commit).
Upgrading to python 3 since 2 is deprecated.

- Changed shebang line from python2 to python3 in all scripts
- Ran scripts with python 2 print statements through 2to3
    - analysis/stim_table.py
    - analysis/DGgrid_analysis_5x5_nikon_SdV.py
    - oscopetools/locally_sparse_noise.py
Autoformatted all python files (outside of oscopetools/sync) using black
with linewidth=80 and leaving quotes as-is. Command: black -S -l 80
analysis/get_eye_tracking.py and oscopetools/get_eye_tracking.py are
identical. This commit removes the analysis version and adjusts the only
place it is imported (analysis/get_all_data.py) accordingly.
The remote-tracking branch 'nauralcodinglab/consolidate' removes
duplicated material between analysis and oscopetools while making sure
no work is lost. This commit integrates these changes into Saskia's
master branch by way of a working branch called cleanup.

This merge commit manually resolves the following conflicts:
- analysis/stim_table.py is modified in Saskia's version and removed in
  nauralcodinglab/consolidate. The only difference (apart from imports)
  is a bugfix in analysis/stim_table.py that addresses an issue that
  caused center-surround trials with a stimulus orientation or zero to
  be dropped. This commit applies the fix to the oscopetools version and
  removes analysis/stim_table.py
- fixes imports and removes commented-out code in
  analysis/get_all_data.py
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