Skip to content

fix: forgot to implement Acquisition.instrument_id merge#1720

Open
dbirman wants to merge 2 commits intodevfrom
1713-acquisition-merge-__add__-needs-to-merge-instrument_id-fields
Open

fix: forgot to implement Acquisition.instrument_id merge#1720
dbirman wants to merge 2 commits intodevfrom
1713-acquisition-merge-__add__-needs-to-merge-instrument_id-fields

Conversation

@dbirman
Copy link
Member

@dbirman dbirman commented Feb 3, 2026

This PR adds the same instrument_id merge logic that we added to Instrument to Acquisition where we forgot to implement it. See #1705

@dbirman dbirman linked an issue Feb 3, 2026 that may be closed by this pull request
@dbirman dbirman requested a review from saskiad February 3, 2026 23:20
@saskiad
Copy link
Collaborator

saskiad commented Feb 4, 2026

I'm a bit confused why we need this. For instrument I get it - we're combining instruments, they have independent instrument metadata. We merge them to make the new metadata.
I don't understand the need to merge instrument ids in the acquisition - that should have already been done.

@dbirman
Copy link
Member Author

dbirman commented Feb 4, 2026

I'm a bit confused why we need this. For instrument I get it - we're combining instruments, they have independent instrument metadata. We merge them to make the new metadata. I don't understand the need to merge instrument ids in the acquisition - that should have already been done.

When you split an instrument into two or more parts you end up generating multiple pairs of acquisition/instrument metadata, each of which has its own instrument_id.

@saskiad
Copy link
Collaborator

saskiad commented Feb 4, 2026

but when it's being used, the instruments are together and the scientists have create a merged instrument file. So shouldn't that be the instrument id they enter into their acqusition?

@dbirman
Copy link
Member Author

dbirman commented Feb 4, 2026

but when it's being used, the instruments are together and the scientists have create a merged instrument file. So shouldn't that be the instrument id they enter into their acqusition?

If a scientist pre-merged their instruments then yes, but if they use the GatherMetadataJob to merge their instruments then no. See https://docs.allenneuraldynamics.org/en/latest/acquire_upload/upload.html#merge-rules. We support this already for vr foraging + fib / ecephys

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.

Acquisition merge (__add__) needs to merge instrument_id fields

2 participants