MetaCell / geppetto-scidash

Geppetto scidash extension
2 stars 1 forks source link

Test creation view - some test classes have the same name but different import path #403

Closed ddelpiano closed 4 years ago

rgerkin commented 4 years ago

Could you give an example?

ddelpiano commented 4 years ago

Sure, if you check on dendrite in the django administration page and go to Test classes, you can see we currently have 2 InputResistanceTest classes.

The first uses: neuronunit.tests.passive.InputResistanceTest

The second uses: neuronunit.tests.druckmann2013.InputResistanceTest

is this expected? Thx!

gidili commented 4 years ago

We also have a way of configuring the ones we don't want not to be there when the db gets populated the first time (there's a yaml or json file somewhere with the generated list of what should get imported)

rgerkin commented 4 years ago

I see what you mean. Yes, this is expected and in general this is not preventable. I could rename the ones in NeuronUnit to avoid collision, but if in the future we want other SciUnit-based packages to be usable with SciDash (like the ones being developed by HBP) we cannot in general guarantee that classes never have the same name. Doing so would also make the class names unwieldy.

I propose that in any location where multiple tests (or model) class names might appear, as in a list, a function be run to check for name collisions. If there is a collision, the colliding entities should be displayed with names one level deeper (e.g. the two InputResistanceTest classes would shows as passive.InputResistanceTest and druckmann2013.InputResistanceTest). Presumably the display text is not actually used in the clicking UI, so this could be done without breaking anything, but of course this can be checked when the unit tests are developed. I recognize that in some views, this will make for long text that could overflow beyond expected areas, but I think that is better than the alternative that the user cannot even tell the difference.

ddelpiano commented 4 years ago

hi @rgerkin , why don't we go directly with the full import path for all the classes? So we display neuronunit.tests.passive.InputResistanceTest & neuronunit.tests.druckmann2013.InputResistanceTest and at least is clearer for the user and in this way we should not have collisions at all. This imply that we make the drop-down menu a bit larger to accomodate the full import path.

gidili commented 4 years ago

@ddelpiano @rgerkin

I would suggest displaying:

[class name] - [full path]

so in our case "InputResistanceTest - neuronunit.tests.passive.InputResistanceTest" etc. So that sorting stays alphabetical on class name

rgerkin commented 4 years ago

Yes, that makes sense. Or maybe [class name] ([full path - class name]), i.e. "InputResistanceTest (neuronunit.tests.passive)" to economize on horizontal space.

zsinnema commented 4 years ago

Screenshot issue 403 scores Screenshot issue 403 detail

zsinnema commented 4 years ago

the class now has the first part of the import path appended, see screenshots

@rgerkin question: do you like to have the full import part or do you want me to remove the part after the last . (dot)?

rgerkin commented 4 years ago

I think it looks good in the screenshot (where you removed the part after the last dot).