Circuit extraction entitycore integration w/o activities #469
Conversation
| ) | ||
| run_validation: bool = False | ||
| do_virtual: bool | list[bool] = Field( | ||
| default=True, |
There was a problem hiding this comment.
Minor detail but could we improve the string name="Do virtual" to something more meaningful? I don't know what the right term might be but something like "Keep projecting nodes as virtual population". Hopefully we can find something snappier hough ;)
Same comment for "Create external" which should be more meaningful for users.
The two descriptions could also be improved, maybe starting with the verb we choose for the title. i.e. "Split ..." rather than "Enable"
There was a problem hiding this comment.
Good point! I improved the names/descriptions, hope they are better not. I was planning to iterate on it in more detail in #347.
There was a problem hiding this comment.
Ok, these are much better. I would suggest these slight iterations on the two descriptions:
-
Additionally extract virtual neurons which target the cells contained in the specified neuron set (together with their connectivity onto the specified neuron set).
-
Convert neurons which are outside of the specified neuron set, but which target the cells contained therein, into a new external population of virtual neurons (together with their connectivity onto the specified neuron set).
But these could be followed up in the next ticket
| " create_external=False,\n", | ||
| " run_validation=False,\n", | ||
| " ),\n", | ||
| " neuron_set=obi.PredefinedNeuronSet(node_set=\"Excitatory\", sample_percentage=[50, 100]),\n", |
There was a problem hiding this comment.
I think the virtual_sources_to_ignore should be made optional.
Currently building the form gives a missing parameter validation issue in this example
There was a problem hiding this comment.
Hmm, weird about the missing parameter validation, did not happen for me as it defaults to () (empty tuple).
In any case, I removed virtual_sources_to_ignore completely for now, since this was supposed to be a quick fix for some problems with virtual populations earlier, but I think it is not needed any more at the moment. And it would complicate the UI/agent quite a bit I believe, since it is a list to be filled in with valid names of virtual source node populations.
So I would suggest to leave the user only the option to completely enable or disable virtual populations (with do_virtual) for the initial version. And we may add more fine grained control of this behavior later, like individual selection of populations to be included, if needed.
There was a problem hiding this comment.
Now I get this error, running this notebook with run_validation=True
|
|
||
| circuit = Circuit( | ||
| name=dest_dir.name, | ||
| name=self.entity(db_client=db_client).name, |
There was a problem hiding this comment.
I guess you've thought it through that its ok for there to be a potential discrepancy between the path and the name? Could you help me think through how the path and name are used in parameter scans?
There was a problem hiding this comment.
So the original idea of the name attribute of Circuit was to add a meaningful (user-readable) name unrelated to the actual path. So it seems natural to me to use the name of the entity for this object here.
Usually, this name is used in the folder names whenever the circuit is part of the scan and "NAME_EQUALS_VALUE" is used (e.g. initialize.circuit=N_10__top_nodes_dim6). However, I just realized that this is not true for CircuitFromID objects, so I added an explicit string representation to the base class EntityFromID in which case the folder name would be something like initialize.circuit=CircuitFromID_18ae1ca7-1adc-45c4-a475-635f93446c0e (before, it was initialize.circuit=CircuitFromID(id_str='18ae1ca7-1adc-45c4-a475-635f93446c0e', type='CircuitFromID') which I think is not so nice).
So the only place I could see where circuit.name (i.e., after staging a CircuitFromID) is actually used is for logging purposes, so it is still nice to have a meaningful name I think. An alternative could be to set the name of the returned Circuit object after staging to the same as of the underlying CircuitFromID object (which won't require db_client), i.e.,
circuit = Circuit(
name=str(self),
...,
)
What do you think?
There was a problem hiding this comment.
Cool, this works well!
There was a problem hiding this comment.
Building the form here works without missing parameters validation error, so I'm confused why it fails in the other notebook
There was a problem hiding this comment.
No sure, for me it did work in both notebooks.
There was a problem hiding this comment.
This notebook works well! Very cool to register an extracted circuit!
A couple of small notes not related to this PR:
-
Because the name of the person who uploaded the extractd circuit + the time of day of the upload don't show, it's difficult to see in the UI whether I successfully uploaded a circuit. I can see the count of the number of circuits incrementing though whilst its running in the UI though :)
-
It's quite slow for such "tiny tiny circuits". Maybe we should ask about parallel registration of assets through entitysdk!
Circuit extraction currently works with the local file system only. This PR extends the functionality so that (a) the input circuit can be retrieved as a
CircuitFromIDfrom entitycore, and (b) the resulting extracted circuit will be registered as a newCircuitentity in entitycore, including the "sonata_circuit" asset and contribution/publication links.Note: The example notebook
A_service_and_entitycore/obi_one_entity_tools/entitysdk_example.ipynbhas been extended to demonstrate the new functionalityRelated issue: #367