add IonChannelRecording #305

AurelienJaquier · 2025-07-23T14:21:22Z

solves https://github.com/openbraininstitute/prod-build-ion-channel-model/issues/11

AurelienJaquier · 2025-07-31T11:49:45Z

This PR is good from my point of view, but I have an error that I really do not know how to solve. It looks like pyright cannot import ElectricalRecordingStimulusRead, but I really do not see why. Also I do not have this error when I run pyright locally.
Do you guys know how to solve this error? @GianlucaFicarelli maybe? I really do not see what is wrong here.

GianlucaFicarelli · 2025-07-31T13:37:14Z

I cannot reproduce the pyright error locally either, so it's difficult to debug.
Probably unrelated, but you could try to merge or rebase on the main branch?

GianlucaFicarelli · 2025-07-31T13:42:22Z

app/db/model.py

+class IonChannelRecording(ElectricalCellRecording):
+    __tablename__ = EntityType.ion_channel_recording.value
+
+    ion_channel: Mapped[IonChannel]


This causes an error when running tests. Maybe you want to add ion_channel_id, and a relationship ion_channel?

… into ion-channel-recording

AurelienJaquier · 2025-08-08T08:02:33Z

Alright @GianlucaFicarelli I have merge the main branch and I have added ion_channel_id and a relationship ion_channel.

… into ion-channel-recording

AurelienJaquier · 2025-08-26T09:23:03Z

@GianlucaFicarelli could I ask for your help with the tests please? I have been trying to fix the sqlalchemy errors, but the number of errors only seem to grow

GianlucaFicarelli · 2025-08-26T10:46:44Z

app/db/model.py

+    __mapper_args__ = {"polymorphic_identity": __tablename__}  # noqa: RUF012
+
+
+class IonChannelRecording(ElectricalCellRecording):


I wonder what is the reason for IonChannelRecording to inherit from ElectricalCellRecording, is it a special case of ElectricalCellRecording?
Alternatively, couldn't it be a separate class?
Or can IonChannelRecording and ElectricalCellRecording have conceptually a common base class?

I have made it inherit from ElectricalCellRecording, because it is indeed a special case of ElectricalCellRecording.
In both the ion channel recording experiments and the 'regular' recording experiments having produced the ElectricalCellRecording data we currently have on the platform, the pipette is applied to a cell (this is named a whole-cell patch clamp experiment).
But in the case of an ion channel recording, the cell has been designed in the laboratory to only express one single type of ion channel, so only currents from this type of ion channel are recorded.
So this is why I made IonChannelRecording inherit from ElectricalCellRecording. I also thought it would be easier (i.e. less code to write down) to just inherit from ElectricalCellRecording.

Although making a common base class for the two classes would also be ok, in my opinion, if you think this structure would be simpler. The base class would simply have all (or most) of ElectricalCellRecording fields.

I would personnally push for IonChannelrecording inheriting from ElectricalCellRecording if that is ok with you @GianlucaFicarelli

@AurelienJaquier to continue and clarify the point, if we use this class/tables hierarchy:
entity -> scientific_artifact -> electrical_cell_recording -> ion_channel_recording
then, when retrieving the list of electrical_cell_recording, also the entries for the ion_channel_recording would be returned (because for each record in the ion_channel_recording table, there is also a record in the electrical_cell_recording table)
Wouldn't be surprising if we return also electrical_cell_recording built from partial data of ion_channel_recording?

Ah I see. In that case, it would be better to have both classes separate, and inheriting from a RecordingBase class or something. We do not want the IonChannelRecordings to show up when we query the ElectricalCellRecordings

* origin/main: Validate scale and build_category in circuit filter (#352) Add admin delete endpoints for all routers (#281) root routes should not require auth, make sure tests reflect this (#351) Specify permission for service admin (#342) Add assets in ValidationResultRead schema (#348) Fix transaction_per_migration in alembic (#346)

GianlucaFicarelli · 2025-09-09T13:40:07Z

app/db/model.py

+
+    label: Mapped[str]
+    gene: Mapped[str]
+    synonyms: Mapped[STRING_LIST]


Some points to decide or clarify:

Should pref_label be made unique?

Is there a validation possible for gene?

For confirmation, should only the service admin be allowed to create a new entry?

Do we have examples for label, gene, synonyms?

@AurelienJaquier

Yes, label (or pref_label) should be unique

only admin service should be allowed to create a new entry

we have the full list of label, gene, synonyms in register_recordings_and_ion_channels.zip in https://github.com/openbraininstitute/prod-build-ion-channel-model/issues/12

We can validate by checking if the data corresponds to what we have in the file I mentioned above

* origin/main: Adds deterministic order fallback by entity id (#350)

GianlucaFicarelli

This can be reviewed

GianlucaFicarelli · 2025-09-11T07:33:19Z

app/db/model.py

+        super_table_args = getattr(super(), "__table_args__", ())
+        # add the index only to the same table where the Mixin is defined, not subclasses
+        attr = getattr(cls, "description_vector", None)
+        if isinstance(attr, MappedColumn):


I cannot find a cleaner way to avoid adding the index to the subclasses, feel free to suggest different options.
This might not work as expected if the mixin is applied to an abstract class, but it should work in our use cases (when the mixin is applied to an intermediate concrete class, or to a final child class).

yeesh, that is a work around, fancy

app/filters/ion_channel_recording.py

eleftherioszisis

Looks good!

mgeplf

LGTM; not sure if you want to squash the alembic versions, or if they need to be 3 separate ones.

GianlucaFicarelli · 2025-09-11T09:07:27Z

LGTM; not sure if you want to squash the alembic versions, or if they need to be 3 separate ones.

I'm thinking to keep them separate because the 1st one is manual, while the other 2 are automatic (I could merge those 2, but still requires some manual copying of the operations in the correct order)

mgeplf · 2025-09-11T09:10:19Z

I'm thinking to keep them separate

Ok, no need.

* origin/main: Add update endpoints for users (#347)

GianlucaFicarelli · 2025-09-11T10:07:35Z

Added new update endpoint introduced in #347

eleftherioszisis

Nice! Thank you for adding it.

* origin/main: Add IonChannel and IonChannelRecording (#305) Add update endpoints for users (#347) Adds deterministic order fallback by entity id (#350) Validate scale and build_category in circuit filter (#352) Add admin delete endpoints for all routers (#281) root routes should not require auth, make sure tests reflect this (#351) Specify permission for service admin (#342) Add assets in ValidationResultRead schema (#348) Fix transaction_per_migration in alembic (#346) Add the ability to filter by name to IonChannelModel (#338)

…-model * origin/main: Add EMCellMesh and EMDenseReconstructionDataset (#339) Add IonChannel and IonChannelRecording (#305) Add update endpoints for users (#347) Adds deterministic order fallback by entity id (#350)

add IonChannelRecording

61fdbb0

AurelienJaquier self-assigned this Jul 23, 2025

AurelienJaquier added 6 commits July 23, 2025 16:23

fix typo

e29d026

lint fix

8beeab8

lint fix

9d060a0

lint fix

4b5e87e

lint fix

87030e1

fix polymorphic identity error

6f5770f

AurelienJaquier marked this pull request as draft July 25, 2025 07:25

AurelienJaquier added 5 commits July 28, 2025 11:21

add IonChannel

bdfb428

lint fix

9f1bc83

lint fix

1a43191

lint fix

386fb8e

lint fix

74c7af5

AurelienJaquier marked this pull request as ready for review July 31, 2025 11:49

GianlucaFicarelli reviewed Jul 31, 2025

View reviewed changes

AurelienJaquier added 3 commits August 4, 2025 09:26

add ion channel id and relationship

88665ad

Merge branch 'main' of https://github.com/openbraininstitute/entitycore…

b66c2dc

… into ion-channel-recording

Merge branch 'main' of https://github.com/openbraininstitute/entitycore…

df8b798

… into ion-channel-recording

AurelienJaquier added 8 commits August 26, 2025 09:25

Merge branch 'main' of https://github.com/openbraininstitute/entitycore…

ab0e8a9

… into ion-channel-recording

fix import

78098e1

fix sqlalchemy.exc.ArgumentError

1c31fbb

solves NoForeignKeysError

842c297

fix id

3e8f3a5

add model_config to IonChannelRecording

0c5c9af

lint fix

adbf9a2

fix config dict

918eeda

GianlucaFicarelli reviewed Aug 26, 2025

View reviewed changes

GianlucaFicarelli reviewed Sep 9, 2025

View reviewed changes

GianlucaFicarelli added 5 commits September 10, 2025 10:25

Update models, endpoints, and add migration

5863795

Add concrete base table electrical_recording

9c5e1ec

Add tests, fix ion_channel filter

d6b0b57

Merge remote-tracking branch 'origin/main' into ion-channel-recording

b67dd71

* origin/main: Adds deterministic order fallback by entity id (#350)

Remove leftover comment

63dec9e

GianlucaFicarelli reviewed Sep 11, 2025

View reviewed changes

GianlucaFicarelli requested review from mgeplf and eleftherioszisis September 11, 2025 07:37

eleftherioszisis reviewed Sep 11, 2025

View reviewed changes

app/filters/ion_channel_recording.py Show resolved Hide resolved

eleftherioszisis approved these changes Sep 11, 2025

View reviewed changes

mgeplf approved these changes Sep 11, 2025

View reviewed changes

GianlucaFicarelli added 2 commits September 11, 2025 11:55

Merge remote-tracking branch 'origin/main' into ion-channel-recording

2297a0a

* origin/main: Add update endpoints for users (#347)

Add update endpoint

f91cbcd

GianlucaFicarelli requested review from eleftherioszisis and mgeplf September 11, 2025 10:07

eleftherioszisis approved these changes Sep 11, 2025

View reviewed changes

GianlucaFicarelli merged commit 0a6018f into main Sep 11, 2025
1 check passed

GianlucaFicarelli deleted the ion-channel-recording branch September 11, 2025 10:13

		__mapper_args__ = {"polymorphic_identity": __tablename__} # noqa: RUF012


		class IonChannelRecording(ElectricalCellRecording):

add IonChannelRecording #305

add IonChannelRecording #305

Uh oh!

Conversation

AurelienJaquier commented Jul 23, 2025

Uh oh!

AurelienJaquier commented Jul 31, 2025

Uh oh!

GianlucaFicarelli commented Jul 31, 2025

Uh oh!

GianlucaFicarelli Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

AurelienJaquier commented Aug 8, 2025

Uh oh!

AurelienJaquier commented Aug 26, 2025

Uh oh!

GianlucaFicarelli Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AurelienJaquier Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GianlucaFicarelli Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

AurelienJaquier Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

GianlucaFicarelli Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AurelienJaquier Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

GianlucaFicarelli left a comment

Choose a reason for hiding this comment

Uh oh!

GianlucaFicarelli Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgeplf Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eleftherioszisis left a comment

Choose a reason for hiding this comment

Uh oh!

mgeplf left a comment

Choose a reason for hiding this comment

Uh oh!

GianlucaFicarelli commented Sep 11, 2025

Uh oh!

mgeplf commented Sep 11, 2025

Uh oh!

GianlucaFicarelli commented Sep 11, 2025

Uh oh!

eleftherioszisis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GianlucaFicarelli Aug 26, 2025 •

edited

Loading

AurelienJaquier Aug 26, 2025 •

edited

Loading

GianlucaFicarelli Sep 9, 2025 •

edited

Loading

GianlucaFicarelli Sep 11, 2025 •

edited

Loading