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.

Sign up for GitHub

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

Jump to bottom

Group part delete propagation #899

Merged
merged 7 commits into from
May 6, 2024
Merged

Group part delete propagation #899

merged 7 commits into from
May 6, 2024

Conversation

samuelbray32
Copy link
Collaborator

Description

Resolves #860

  • Makes new table class SpyglassGroupPart
  • Allows propagation of delete from upstream of part to downstream of master
  • Changes definition of parts in PositionGroup, SortedSpikesGroup, UnitWaveformFeaturesGroup

Checklist:

  • This PR should be accompanied by a release: no
  • (If release) I have updated the CITATION.cff
  • I have updated the CHANGELOG.md
  • I have added/edited docs/notebooks to reflect the changes

CBroz1
CBroz1 reviewed Mar 27, 2024
View reviewed changes
Copy link
Member

@CBroz1 CBroz1 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @samuelbray32 !

I think this establishes Group as a new table type beyond the level of convention, and therefore worth inclusion in this doc. Are you able to add that as part of this PR?

src/spyglass/utils/__init__.py Outdated Show resolved Hide resolved
src/spyglass/utils/dj_mixin.py Outdated Show resolved Hide resolved
@edeno
Copy link
Collaborator

edeno commented Mar 27, 2024

Do you think that we need a new table type or is this something that could be detected?

@CBroz1
Copy link
Member

CBroz1 commented Mar 27, 2024 •
edited
Loading

Do you think that we need a new table type or is this something that could be detected?

I took from the description above that Group was an implied tables type that we had yet to formalize. If it's conceptually distinct, my gut was to describe it as such.

Reviewing #860 more closely, it looks like is position_group__position was falsely identified as a merge table and passed to _commit_merge_deletes when it shouldn't be. I think this could instead be fixed by revising how merge tables are identified here (which already needs some work anyway bc I need to try/catch the networkx 'not in graph' error mentioned in #886). It checks that merge_id is in the heading, but should ensure that that is the only pk.

The mixin could also do a better job enforcing that non-merge tables do not meet the criteria I use to ID merges

@CBroz1 CBroz1 mentioned this pull request Mar 29, 2024
Closed
@CBroz1 CBroz1 marked this pull request as draft April 17, 2024 21:04
@edeno edeno added the infrastructure Unix, MySQL, etc. settings/issues impacting users label Apr 19, 2024
@CBroz1 @samuelbray32 @edeno
Part delete edits (#946)
2d9cc73 
* Add spyglass version to created analysis nwb files (#897)

* Add sg version to created analysis nwb files

* update changelog

* Change existing source script to spyglass version (#900)

* Add pynapple support (#898)

* Preliminary code

* Add retrieval of file names

* Add get_nwb_table function

* Update docstrings

* Update CHANGELOG.md

* Hot fixes for clusterless `get_ahead_behind_distance` and `get_spike_times` (#904)

* Squeeze results

* Make method and not class method

* Update CHANGELOG.md

* fix bugs in fetch_nwb (#913)

* Check for entry in merge part table prior to insert (#922)

* check for entry in merge part table prior to insert

* update changelog

* Kachery fixes (#918)

* Prioritize datajoint filepath for getting analysis file abs_path

* remove deprecated kachery tables

* update changelog

* fix lint

---------

Co-authored-by: Samuel Bray <[email protected]>
Co-authored-by: Eric Denovellis <[email protected]>

* remove old tables from init (#925)

* Fix improper uses of strip (#929)

Strip will remove leading characters

* Update CHANGELOG.md

* Misc Issues (#903)

* #892

* #885

* #879

* Partial address of #860

* Update Changelog

* Partial solve of #886 - Ask import

* Fix failing tests

* Add note on order of inheritace

* #933

* Could not replicate fill_nan error. Reverting except clause

* Export logger (#875)

* WIP: rebase Export process

* WIP: revise doc

* ✅ : Generate working export script

* Cleanup: Expand notebook, migrate export process from graph class to export

* Revert dj_chains related edits

* Update changelog

* Revise doc

* Address review comments #875

* Remove walrus in  eval

* prevent log on preview

* Fix arg order on fetch, iterate over restr

* Add upstream analysis files during cascade. Address false positive fetch

* Avoid regen file list on revisit node

* Bump Export.Table.restr to mediumblob

* Revise Export.Table uniqueness to include export_id

* Spikesorting quality of life helpers (#910)

* add utitlity function for finding spikesorting merge ids

* add option to select v1 sorts that didn't go through artifact detection

* add option to return merge keys as dicts for future restrictions

* Add tool to get brain region and electrode info for a spikesorting merge id

* update changelog

* style cleanup

* style cleanup

* fix restriction bug for curation_id

* account for change or radiu_um argument name in spikeinterface

* only do joins with metric curastion tables if have relevant keys in the restriction

* Update tutorial to use spikesorting merge table helper functions

* fix spelling

* Add logging of AnalysisNwbfile creation time and file size (#937)

* Add logging for any func that creates AnalysisNwbfile

* Migrate create to top of respective funcs

* Use pathlib for file size. Bump creation time to top of  in spikesort

* Clear pre_create_time on create

* get/del -> pop

* Log when file accessed (#941)

* Add logging for any func that creates AnalysisNwbfile

* Fix bug on empty delete in merge table (#940)

* fix bug on empty delete in merge table

* update changelog

* fix spelling

---------

Co-authored-by: Chris Brozdowski <[email protected]>

* Remove master restriction

* Part delete takes restriction from self

---------

Co-authored-by: Samuel Bray <[email protected]>
Co-authored-by: Eric Denovellis <[email protected]>
Co-authored-by: Samuel Bray <[email protected]>
Co-authored-by: Eric Denovellis <[email protected]>
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@edeno edeno marked this pull request as ready for review April 25, 2024 20:58
@edeno edeno requested a review from CBroz1 April 25, 2024 20:59
edeno
edeno requested changes Apr 25, 2024
View reviewed changes
Copy link
Collaborator

@edeno edeno left a comment

Choose a reason for hiding this comment

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

There seem to be many more tables that have SpyglassMixin, dj.Part:

  • SpatialSeries, PosObject, Shank, Electrode, LFPElectrode, LFPBandElectrode, LabMemberInfo, LabTeamMember, RippleLFPElectrode, DataAcquisitionDevice, etc.

Should these also be replaced?

@edeno
Copy link
Collaborator

edeno commented May 6, 2024

It seems like it will take awhile to for datajoint to merge the change on their end so we should not wait. It seems like all this needs is updates to the other tables with SpyglassMixin, dj.Part and it should be good to go.

@samuelbray32
Copy link
Collaborator Author

@edeno, sorry I missed your previous comment. I don't think all the examples listed functionally need the changes to their delete function. It's not needed for every part table, but ones where the part is a child of a likely-to-be deleted table that the parent is not (see image attached). This motif showed up in the Group tables, which are useful and might be worth more formally defining. That was my reasoning for the original GroupPart name.

image

That said, I don't think this functionality hurts other part tables besides reducing restrictions on how users can interact and delete from database. If we want to keep it general for all part tables then I can quickly add it to those other table definitions.

@edeno edeno merged commit 435b0f3 into master May 6, 2024
7 checks passed
@edeno edeno deleted the group_part_delete branch May 6, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edeno edeno edeno approved these changes

@CBroz1 CBroz1 Awaiting requested review from CBroz1

Assignees
No one assigned
Labels
infrastructure Unix, MySQL, etc. settings/issues impacting users
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failed delete propagation through PositionGroup
3 participants
@samuelbray32 @edeno @CBroz1