Open arnaudon opened 1 year ago
codecov[bot]
commented
1 year ago Merging #61 (29a57c3) into main (9d74824) will decrease coverage by
0.04%. The diff coverage is100.00%.:exclamation: Current head 29a57c3 differs from pull request most recent head 301455f. Consider uploading reports for the commit 301455f to get more accurate results
arnaudon
commented
1 year ago @adrien-berchet , when you have 5min, if you can take a look at this to see if the general implementation makes sense. I will then use context to modify the prob() function.
adrien-berchet
commented
1 year ago @adrien-berchet , when you have 5min, if you can take a look at this to see if the general implementation makes sense. I will then use context to modify the prob() function.
Looks nice!
arnaudon
commented
1 year ago @lidakanari @adrien-berchet , this can be reviewed as well, again, it is experimental, so not fully tested
codecov-commenter
commented
8 months ago Attention: Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.
Project coverage is 97.79%. Comparing base (
38f8e66) to head (83b9368).
| Files with missing lines | Patch % | Lines |
|---|---|---|
| neurots/generate/section.py | 90.00% | 1 Missing and 1 partial :warning: |
eleftherioszisis
commented
1 month ago I am concerned about this PR. There are a lot of changes affecting the core functionality in many places and almost no tests to test the new functionality thoroughly. In addition, there are no tests for the expected behavior of the algorithm that should validate the sampling algorithm behavior.
By just looking at the review, I doubt any of us can guarantee that these changes are not breaking or introducing unexpected behavior.
And on a different note, is there a performance benchmark for this new approach? accept/reject is usually quite slower and synthesis already takes quite some time when considered from a large-scale point of view. Not a priority, I know, but a ballpark estimation to make sure we don't introduce a 100x slowdown would be nice.
arnaudon
commented
1 month ago I'm not sure which 'core functionalities' you are mentioning? its all under if self.context is not None no?
adrien-berchet
commented
1 month ago What about moving this feature in a mixin so it would not change anything on the usual stuff and it's still possible to use it for external applications?
eleftherioszisis
commented
1 month ago I'm not sure which 'core functionalities' you are mentioning? its all under
if self.context is not Noneno?
I see changes in section.py, tree.py, orientations.py. All these are core modules.
Also, other parts of the code use a context too. For instance: https://github.com/BlueBrain/NeuroTS/blob/9e586f72e23fe7d936571f1f97ef47f15c88338e/neurots/astrocyte/context.py#L54
Maybe it doesn't matter, but keep in mind that the generic classes should not cover specific use cases encountered only in neurons. This is what derived classes are for.
The used context should have an interface rather than accessing keys at random in different parts of the code so that users/devs can understand what they are expected to pass. This can be as simple as having a json schema or a class that represents that context, whichever is preferable. Having an undefined dictionary that could have anything in it, makes the code very hard to use.
What about moving this feature in a mixin so it would not change anything on the usual stuff and it's still possible to use it for external applications?
Not sure. If you wish we could discuss it in more detail.
arnaudon
commented
1 month ago What would be your suggestion to improve then? I don't mind leaving this open for ever, it does the job I need it to do with the other open PR on region-grower, which is almost over with bbp end. I can surely add a schema here, which I would have to overwrite in region-grower (as the other schemas) so I can have more freedom from that side. This is supposed to be a 'hidden' feature, not a core feature, but it enters it at the 'core' of the code, its not my fault ;) If I recall coding this 3y ago or so, i tried to be minimalistic in the implementation, but I'm open to suggestions.
eleftherioszisis
commented
1 month ago What would be your suggestion to improve then? I don't mind leaving this open for ever, it does the job I need it to do with the other open PR on region-grower, which is almost over with bbp end. I can surely add a schema here, which I would have to overwrite in region-grower (as the other schemas) so I can have more freedom from that side. This is supposed to be a 'hidden' feature, not a core feature, but it enters it at the 'core' of the code, its not my fault ;) If I recall coding this 3y ago or so, i tried to be minimalistic in the implementation, but I'm open to suggestions.
I would say mainly test that all the new parts of the code work as expected and produce the expected sampling.
And yeah, having a schema for the context will help make things standardized. Why would it be a hidden feature though?
arnaudon
commented
1 month ago I can try to add a couple more tests, but this code is not meant to be use 'directly'. There is no probability function implemented here, it has to be passed by a user externally via the context. The 'success' of the sampling will very much depend on this proba function, as you know. It is hidden in this sense: its not easy to use at all, and very experimental in term of writing a good probability function to do what the user needs. In this case, the user is me only, via region-grower (not OSS). Also, hidden in the sense: unless you know the specific way to access this part of the code (via me, or region-grower), you will not find it ;)
arnaudon
commented
1 month ago @adrien-berchet , I'll look at you suggestion soon, clean up a few things and check the tests maybe later this week
adrien-berchet
commented
1 month ago And what about just improving how external applications can customize the tree and section growers of NeuroTS? So we could just define a new custom grower in region-grower with a specific behavior and pass it to NeuroTS.
eleftherioszisis
commented
1 month ago And what about just improving how external applications can customize the tree and section growers of NeuroTS? So we could just define a new custom grower in
region-growerwith a specific behavior and pass it to NeuroTS.
Why not specify that custom grower here? It would be easier to ensure with tests that changes in NeuroTS are compatible with the custom growers.
This is what I did for astrocytes for example.
adrien-berchet
commented
1 month ago Why not specify that custom grower here? It would be easier to ensure with tests that changes in NeuroTS are compatible with the custom growers.
This is what I did for astrocytes for example.
It would be just to make it clear that this feature is specific to region-grower, not a core feature. For example in axon-synthesis I defined another custom grower (with another method to handle boundaries).
But anyway, when no context is given this PR only adds 2 if statements so it should not have any impact on the performance. So I think that we should mainly decide if this feature should be part of the core features or just optional and focus on improving the test coverage and the readability of the code.
arnaudon
commented
1 month ago tests are fine locally, weird weird
This PR adds an accept/reject mechanisms to allow for external modification of section and trunk growths via the generic
contextobject.