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

Fixing #264: Raise exceptions on incommensurate times #265

Merged
merged 6 commits into from Mar 11, 2016

Conversation

Projects
None yet
3 participants
@heplesser
Contributor

heplesser commented Mar 8, 2016

Simulate now raises exception if simulation time is not multiple of the resolution. SetStatus on devices do the same if start, stop, origin are not multiples of the resolution. Some tests needed minor adaptation.

@jougs @apeyser Would you take a look?

Fixing #264: Simulate now raises exception if simulation time is not …
…multiple of the resolution. SetStatus on devices do the same if start, stop, origin are not multiples of the resolution. Some tests needed minor adaptation.
@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Mar 8, 2016

Why the initialization? Isn't v just a temporary variable to reference times in a dictionary?

Why the initialization? Isn't v just a temporary variable to reference times in a dictionary?

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 10, 2016

Owner

@apeyser I guess it is not strictly necessary, and I added that while hunting a bug that had entirely different causes, but it does not hurt either, does it?

Owner

heplesser replied Mar 10, 2016

@apeyser I guess it is not strictly necessary, and I added that while hunting a bug that had entirely different causes, but it does not hurt either, does it?

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Mar 10, 2016

@heplesser I guess it's a matter of taste. I took it as an indication that it was an input, when it's only an output variable, and hunted for where the initialization was necessary. If the following chunks are wrapped up into a convenience function, it goes away altogether.

@heplesser I guess it's a matter of taste. I took it as an indication that it was an input, when it's only an output variable, and hunted for where the initialization was necessary. If the following chunks are wrapped up into a convenience function, it goes away altogether.

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Mar 8, 2016

Can't we wrap all the settings code into a local static function with three parameters, v, &timeref, name? The all look exactly the same to me.

Can't we wrap all the settings code into a local static function with three parameters, v, &timeref, name? The all look exactly the same to me.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 8, 2016

Contributor

I think the code is very nice. 👍 once Travis is also OK with it.

Contributor

jougs commented Mar 8, 2016

I think the code is very nice. 👍 once Travis is also OK with it.

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Mar 8, 2016

Can this be infinite?

Can this be infinite?

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 11, 2016

Owner

I don't think you could enter infinity from SLI, but you could probably from Py, and we should avoid that. I will add addition tests, also against negative sim time.

Owner

heplesser replied Mar 11, 2016

I don't think you could enter infinity from SLI, but you could probably from Py, and we should avoid that. I will add addition tests, also against negative sim time.

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Mar 8, 2016

This should fail as long as TICS_PER_MS > 2, which is by default 1000. Should we comment or test this condition -- that it works when TICS_PER_MS >=2 ?

This should fail as long as TICS_PER_MS > 2, which is by default 1000. Should we comment or test this condition -- that it works when TICS_PER_MS >=2 ?

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 11, 2016

Owner

The test value 1.5 should be exactly representable in tics, so we should require TICS_PER_MS to be a multiple of 2.

Owner

heplesser replied Mar 11, 2016

The test value 1.5 should be exactly representable in tics, so we should require TICS_PER_MS to be a multiple of 2.

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Mar 8, 2016

So this rounds down from stimtime + 2 ms to the nearest grid, correct? Shouldn't this round up to the nearest grid, or is the 2ms supposed to be large enough to always go to the next biggest grid time for all test cases, which is then sufficient for all spikes to arrive at spike detector? If it's the latter, it would probably be good to add a comment by the tested cases in the assert or die regarding what is the maximum testable case value (0.1? 0.2?)

So this rounds down from stimtime + 2 ms to the nearest grid, correct? Shouldn't this round up to the nearest grid, or is the 2ms supposed to be large enough to always go to the next biggest grid time for all test cases, which is then sufficient for all spikes to arrive at spike detector? If it's the latter, it would probably be good to add a comment by the tested cases in the assert or die regarding what is the maximum testable case value (0.1? 0.2?)

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 8, 2016

Owner

@apeyser You are right, I hope the improvement I added right now addresses your concerns.

Owner

heplesser commented on 9b9153a Mar 8, 2016

@apeyser You are right, I hope the improvement I added right now addresses your concerns.

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 11, 2016

Owner

@apeyser I just pushed several new commits that hopefully address your remaining concerns.

Owner

heplesser replied Mar 11, 2016

@apeyser I just pushed several new commits that hopefully address your remaining concerns.

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Mar 10, 2016

Invariants: simtime & res are floats (then it works correctly and the same for py2 & py3)

Invariants: simtime & res are floats (then it works correctly and the same for py2 & py3)

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 11, 2016

Owner

I'll add explicit conversion to be safe.

Owner

heplesser replied Mar 11, 2016

I'll add explicit conversion to be safe.

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Mar 11, 2016

Contributor

👍 I'll merge after ci.

Contributor

apeyser commented Mar 11, 2016

👍 I'll merge after ci.

heplesser added a commit that referenced this pull request Mar 11, 2016

Merge pull request #265 from heplesser/fix264_incommensurate_simtime_…
…check

Fixing #264: Raise exceptions on incommensurate times

@heplesser heplesser merged commit b7b1f97 into nest:master Mar 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@heplesser heplesser deleted the heplesser:fix264_incommensurate_simtime_check branch Mar 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment