Skip to content

Conversation

@Matyasz
Copy link
Contributor

@Matyasz Matyasz commented Oct 13, 2020

Overview:

This is a response to the LIMS issue that appeared this morning. LIMS Job 1056188052 in BEHAVIOR_OPHYS_WRITE_NWB_QUEUE failed because imaging_plane_group was not in the data. This is expected, so we altered the code to account for non-mesoscope data, which will not have this parameter.

Addresses:

Type of Fix:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing
    functionality to not work as expected)
  • Documentation Change

Solution:

get_imaging_plane_group will now return None if not dealing with mesoscope data

Changes:

  • get_imaging_plane_group is not a ry/except, not just a return statement
  • Version is updated to 2.3.1

Validation:

Screenshots:

Unit Tests:

Script to reproduce error and fix:

Configuration details:

I ran the tests locally and they passed

Checklist

  • My code follows
    Allen Institute Contribution Guidelines
  • My code is unit tested and does not decrease test coverage
  • I have performed a self review of my own code
  • My code is well-documented, and the docstrings conform to
    Numpy Standards
  • I have updated the documentation of the repository where
    appropriate
  • The header on my commit includes the issue number
  • My Pull Request has the latest AllenSDK release candidate branch
    rc/x.y.z as its merge target
  • My code passes all AllenSDK tests

Notes:

@djkapner
Copy link
Contributor

does this need to go straight to master, or can it go to rc/2.4.0 and merge in 1-3 weeks?

@kschelonka
Copy link
Contributor

LGTM but we should consider this a temporary workaround until we have a more in-depth discussion about data architecture and data flow.

@kschelonka
Copy link
Contributor

@djkapner It's failing WRITE_NWB jobs on LIMS right now because it's not a field in the input json. Would be best to merge soon.

@Matyasz Matyasz force-pushed the bugfix-LIMS_jobID_1056188052 branch from 392bd1c to 90c94d3 Compare October 13, 2020 22:09
@codecov-io
Copy link

codecov-io commented Oct 13, 2020

Codecov Report

Merging #1749 into master will decrease coverage by 0.00%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1749      +/-   ##
==========================================
- Coverage   36.16%   36.16%   -0.01%     
==========================================
  Files         346      346              
  Lines       33814    33817       +3     
==========================================
  Hits        12229    12229              
- Misses      21585    21588       +3     
Impacted Files Coverage Δ
...k/brain_observatory/behavior/write_nwb/__main__.py 42.05% <0.00%> (-1.22%) ⬇️
allensdk/__init__.py 68.96% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb96fc9...fe71cd8. Read the comment docs.

djkapner
djkapner previously approved these changes Oct 13, 2020
…scope data.

Updated the changelog and index.rst for bugfix deployment.
@Matyasz Matyasz force-pushed the bugfix-LIMS_jobID_1056188052 branch from 616453e to fe71cd8 Compare October 13, 2020 23:19
@njmei njmei requested a review from djkapner October 13, 2020 23:47
@Matyasz Matyasz merged commit 7dd9dfe into master Oct 13, 2020
@Matyasz Matyasz deleted the bugfix-LIMS_jobID_1056188052 branch October 14, 2020 18:23
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.

6 participants