MetaCell / sckan-composer

0 stars 0 forks source link

Feature/sckan 275 #250

Closed afonsobspinto closed 3 months ago

afonsobspinto commented 3 months ago

Closes https://metacell.atlassian.net/browse/SCKAN-275

image image image image

image image

afonsobspinto commented 3 months ago

I think it might make more sense to have anatomical entities as an abstract model and have as children:

afonsobspinto commented 3 months ago

I updated the description of the PR accordingly to the most recently changes.

For tracking purposes, this is what the original description was: - Updates composer model to have both region and layer as anatomical entities - Update neuronDM script to retain region + layer information - Update ingest statements command to create region and layers when needed. ![image](https://github.com/MetaCell/sckan-composer/assets/19196034/4535f47b-18d2-4e5d-80a7-4c359308e928) ![image](https://github.com/MetaCell/sckan-composer/assets/19196034/65ae3b50-b996-4e79-adae-593f4c0a10db) ![image](https://github.com/MetaCell/sckan-composer/assets/19196034/86571fd9-fdba-4149-9ce8-b46351c4dd59)
afonsobspinto commented 3 months ago

@zsinnema I did not had the time to try any of the speed improvement suggestions on the admin page yet. Would it be possible to merge this PR only with the usability changes so that @ddelpiano can test it?

zsinnema commented 3 months ago

@afonsobspinto imo this is not mergable, see the attached video. In this video I open a (one) ConnectivityStatement in the admin using this link https://127.0.0.1:8000/admin/composer/connectivitystatement/296/change/ (I ingested the dev database)

https://github.com/MetaCell/sckan-composer/assets/124044/843f6f1f-0933-4856-a34e-9e519e5317a3

afonsobspinto commented 3 months ago

@zsinnema Do you believe this is not mergeable do to the high amount of queries? If that's so, I agree with you that it's something that could be improved but my point is that it's not something that is responsibility of this PR. Have a look at the SQL query count in both dev and local with dev database:

Dev: image

Local with dev database: image

zsinnema commented 3 months ago

@afonsobspinto herewith a video with the dev db on my local machine and git branch develop. as you can see the amount of queries is a "million" times less

https://github.com/MetaCell/sckan-composer/assets/124044/53d69ff1-f3aa-4afc-956f-05ec6e00d488

zsinnema commented 3 months ago

@ddelpiano The performance decreased is caused by the chanegs done in this PR/feature. Therefore I classify the implementation as being not mergable. I understand that the client also needs to see things/progress so the question is: how big is the is the performance decrease for the usability of the application. In the past we faced a perf decrease and then it was a big thing. Of course we could say that the performance decrease is a "bug". But the thing is that we already know the bug and the PR isn't merged yet.

Up to you if you think we can merge it or not

afonsobspinto commented 3 months ago

@zsinnema I can't replicate the behaviour of having such high amount of queries when running with this branch with the dev database:

https://github.com/MetaCell/sckan-composer/assets/19196034/d32a065d-898a-4aab-9355-3a08d2305360

This is what I did:

zsinnema commented 3 months ago

@afonsobspinto hmmm after I pulled again from Git the many queries disappeared... weird