Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Different score ReducedModel/InputResistanceTest #219

Open
NeZanyat opened this issue Apr 24, 2019 · 9 comments
Open

Different score ReducedModel/InputResistanceTest #219

NeZanyat opened this issue Apr 24, 2019 · 9 comments

Comments

@NeZanyat
Copy link
Contributor

NeZanyat commented Apr 24, 2019

Different score for couple:

ReducedModel("https://raw.githubusercontent.com/scidash/neuronunit/dev/neuronunit/models/NeuroML2/LEMS_2007One.xml") (results calculated with "timestep":0.00005, "length":0.3,)

InputResistanceTest ({"n": "1", "std": "25", "mean": "100"} \ {"dt": "1", "tmax": "10"})

Insufficient data score - One of the input values was NaN

Reference link - https://github.com/scidash/neuronunit/blob/dev/neuronunit/examples/model_zoo/models-1.ipynb

@rgerkin
Copy link
Contributor

rgerkin commented Apr 24, 2019

Are you saying that one of the prediction values is NaN or one of the observation values is NaN? Why doesn't the observation provided to the InputResistanceTest have units? And where are you getting the dt and tmax from (and why are they different from the values you used to simulated the model)? Is your backend using them? @gidili

@gidili
Copy link

gidili commented Apr 25, 2019

Hi @rgerkin all good points - we are doing more troubleshooting today and will get back later on in the day. dt and tmax should be the same and they are user input fields in our case (that's where they are coming from). We tried with same values and still getting NaN errors during score calculation. We are now doing step by step comparison between running with notebook and with the other stack to see where we diverge. More to come.

@gidili
Copy link

gidili commented Apr 29, 2019

Update on this @rgerkin - we are still on this, made some progress on troubleshooting but still in the weeds to understand the origin of the NaN, seems to be narrowed down to either results values arrays or differences between setting sciunit cache vs sciunit handling everything internally (triggered when doing .judge). We are also trying to use the jNeuroML backend in Geppetto but it really should not make a difference as it also uses neuron under the hood but we are trying to cover all the bases.

More to come today and tomorrow but I think it's safe to expect we will not have a demo ready for thursday's hackathon (since this is the only example that we can use) at this point as we are still in the unknown integration realm unfortunately.

@gidili
Copy link

gidili commented Apr 29, 2019

@rgerkin also I've noticed you are running these examples on the dev branch. We are running on the dash branch, would be good to merge everything and double check everything works the same there for you as well.

@rgerkin
Copy link
Contributor

rgerkin commented Apr 29, 2019

@gidili Dev has everything Dash has, and a bunch of other things. I should have kept them separate but I think I accidentally did some Dash work on Dev branch a few weeks ago. I could cherry-pick commits back to keep them separate, but now is as good a time as any to pull all of Dev into Dash.

Can we make our Tuesday meeting focused on tackling the bug you are experiencing?

@gidili
Copy link

gidili commented Apr 30, 2019

@rgerkin we figured it out. Initially the problem there was that no default was provided for dt and tmax (or if so we couldn't find it) and we had to extract the correct timestep and tmax from the results in the notebook, that bit was easy to figure out but was the first hurdle.

General question: Is there some global defaults you are using in neuronunit for dt and tmax if the test doesn't provide them and if so where we can grab them?

Kept digging further and we are getting results now and they are different from those produced by neuronunit. Looking more into the model and the test and we were expecting inputs (current in this case) to be injected from the lems / neuroml file only from previous conversations we had but the InputResistanceTest is injecting current so that's causing results to be different and consequently the score (because we are not injecting anything if it's not in the model already).

The good news is that score calculation seem to work ok for those results now, but now we need some way to tell if the test will inject current and either 1) add it to the lems or neuroml file if that's the case or 2) assume that current is always injected from the model itself and ignore the test (this is what I thought we were doing but I can see how it's not ideal).

We can discuss this today.

After we sort this stuff out we will write some documentation to make it clear how test and model classes need to be written in neuronunit (which attributes are required for scidash to automatically pick stuff up etc) so that they will work with scidash as new test/model classes are added in the future by the community of curators.

@gidili
Copy link

gidili commented Apr 30, 2019

@rgerkin about moving to dev, sounds good, but even though everything that was in dash is now in dev there might be stuff in dev that wasn't in dash that breaks stuff but hopefully not, we shall see anyway, this needed to happen at some point but I would suggest when we release scidash 1.0 you tag a version of dev or however you wanna do it so the app can keep using that regardless of what happen on the dev branch.

@rgerkin
Copy link
Contributor

rgerkin commented May 7, 2019

@gidili At some point this summer I will get the chance to push for the next sciunit and neuronunit release (for which I will need to increase code coverage and add more examples and documentation). I don't expect it to be much different than what is there now, but that is what I'd like to tag scidash 1.0 against.

Is it possible to trigger scidash integration tests every time a commit is made to sciunit or neuronunit (or alternatively just weekly against the latest version of the major branches of each)?

@gidili
Copy link

gidili commented May 7, 2019

@rgerkin what we thinking to do for now is take a snapshot of sciunit/neuronunit development (via tag) and keep using that for the released version of scidash.

Upon releasing new versions we can create branches targeting new releases and the builds will run unit tests. We don't have any integration tests so far and unclear we are going to have time to write loads as we're slightly behind the last couple of weeks, but unit tests should be enough to cover compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants