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

Removes dependency on hardcoded acronym-id map and strips subregion from acronym #2401

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

aamster
Copy link
Contributor

@aamster aamster commented Apr 29, 2022

Addresses #2396

VCN used a hardcoded acronym-id map. This map does not include all regions, and caused the unit to be excluded when we tried to look up region not in this map. Also, the structure id is not needed, since it is just an id internal to lims, and hence the map is not needed, so it is removed.

This PR also adds ability to strip the subregion from the acronym, since as Corbett pointed out, the subregion is beyond annotation accuracy.

@aamster aamster marked this pull request as ready for review April 29, 2022 18:56
@aamster aamster force-pushed the structure_acronym branch 2 times, most recently from fae4cda to 5745f9e Compare April 29, 2022 20:29
Copy link
Contributor

@danielsf danielsf left a comment

Choose a reason for hiding this comment

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

Two comments:

One is a nice to have (explicitly breaking up that **{k: v for items...} call in _channels.py

The other one I think might be an actual change request (unless you've done some test to make sure we are still compatible with the VCN sessions)

allensdk/brain_observatory/ecephys/_channels.py Outdated Show resolved Hide resolved
allensdk/brain_observatory/ecephys/probes.py Show resolved Hide resolved
@aamster aamster force-pushed the structure_acronym branch 2 times, most recently from 85608e8 to 440453c Compare April 29, 2022 21:44
…since it is an id internal to LIMS;

Adds ability to strip subregion from acronym
@aamster aamster force-pushed the structure_acronym branch from 440453c to d743d34 Compare April 29, 2022 21:50
@danielsf
Copy link
Contributor

@aamster
I think we are about to break VCN, just not how I thought

from allensdk.brain_observatory.ecephys.ecephys_project_cache import (
    EcephysProjectCache)
import os
import hashlib
manifest_path = '/allen/aibs/informatics/danielsf/scratch/ecephys_manifest.json'
if os.path.exists(manifest_path):
    os.unlink(manifest_path)

cache = EcephysProjectCache.from_warehouse(manifest=manifest_path)
session = cache.get_session_data(session_id=719161530)
session.units

fails with

  File "test_warehouse.py", line 11, in <module>
    session.units
  File "/home/scott.daniel/AllenSDK/allensdk/core/lazy_property/lazy_property_mixin.py", line 13, in __getattribute__
    curr_attr = super(LazyPropertyMixin, self).__getattribute__(name)
  File "/home/scott.daniel/AllenSDK/allensdk/brain_observatory/ecephys/ecephys_session.py", line 227, in units
    return self._units.drop(columns=['width_rf',
  File "/home/scott.daniel/AllenSDK/allensdk/core/lazy_property/lazy_property_mixin.py", line 15, in __getattribute__
    return curr_attr.__get__(curr_attr)
  File "/home/scott.daniel/AllenSDK/allensdk/core/lazy_property/lazy_property.py", line 20, in __get__
    self.value = self.calculate()
  File "/home/scott.daniel/AllenSDK/allensdk/core/lazy_property/lazy_property.py", line 30, in calculate
    result = self.api_method(*self.args, **self.kwargs)
  File "/home/scott.daniel/AllenSDK/allensdk/brain_observatory/ecephys/ecephys_session_api/ecephys_nwb_session_api.py", line 158, in get_units
    isi_violations_maximum=self.isi_violations_maximum
  File "/home/scott.daniel/AllenSDK/allensdk/brain_observatory/ecephys/probes.py", line 114, in get_units_table
    ) for p in self.probes
  File "/home/scott.daniel/AllenSDK/allensdk/brain_observatory/ecephys/probes.py", line 114, in <listcomp>
    ) for p in self.probes
  File "/home/scott.daniel/AllenSDK/allensdk/brain_observatory/ecephys/_channels.py", line 60, in to_dataframe
    channels = [channel.to_dict()['channel'] for channel in self.value]
  File "/home/scott.daniel/AllenSDK/allensdk/brain_observatory/ecephys/_channels.py", line 60, in <listcomp>
    channels = [channel.to_dict()['channel'] for channel in self.value]
  File "/home/scott.daniel/AllenSDK/allensdk/core/_data_object_base/data_object.py", line 106, in to_dict
    properties = _get_keys_and_values(base_value=value)
  File "/home/scott.daniel/AllenSDK/allensdk/core/_data_object_base/data_object.py", line 89, in _get_keys_and_values
    for name, value in base_value._get_properties().items():
  File "/home/scott.daniel/AllenSDK/allensdk/core/_data_object_base/data_object.py", line 139, in _get_properties
    return {name: getattr(self, name) for name in props}
  File "/home/scott.daniel/AllenSDK/allensdk/core/_data_object_base/data_object.py", line 139, in <dictcomp>
    return {name: getattr(self, name) for name in props}
  File "/home/scott.daniel/AllenSDK/allensdk/brain_observatory/ecephys/_channel.py", line 78, in manual_structure_acronym
    if self._strip_structure_subregion \
AttributeError: 'float' object has no attribute 'split'

vbn_2022_dev does not seem to have this problem.

In the interest of moving forward with the users we have, I'm okay merging this ticket. Just open a ticket to fix this problem. We can address that before merging vbn_2022_dev into main.

Copy link
Contributor

@danielsf danielsf left a comment

Choose a reason for hiding this comment

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

Let's merge this and fix the VCN problem later

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