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

Fix #740 by representing times in steps in step_current_generator. #815

Merged
merged 4 commits into from Aug 30, 2017

Conversation

Projects
None yet
3 participants
@heplesser
Contributor

heplesser commented Aug 24, 2017

@appukuttan-shailesh Could you be the second reviewer for this PR?

@heplesser heplesser added this to the NEST 2.12.1 milestone Aug 24, 2017

@heplesser heplesser requested a review from jougs Aug 24, 2017

@appukuttan-shailesh

This comment has been minimized.

Show comment
Hide comment
@appukuttan-shailesh

appukuttan-shailesh Aug 25, 2017

Contributor

Sure, I can take this up.

Contributor

appukuttan-shailesh commented Aug 25, 2017

Sure, I can take this up.

@appukuttan-shailesh

This worked well for all the scenarios that I tested. Just minor suggestions regarding the error messages.

{
std::stringstream msg;
msg << "step_current_generator: Time point " << t
<< " is not representable in current resolution.";

This comment has been minimized.

@appukuttan-shailesh

appukuttan-shailesh Aug 28, 2017

Contributor

Currently if the user enters a negative timestamp (which ideally shouldn't be the case), say -0.2, then the error message returned is Time point -0.2 is not representable in current resolution. We could possibly have a check for timestamps being negative, and throw a more appropriate warning message.

@appukuttan-shailesh

appukuttan-shailesh Aug 28, 2017

Contributor

Currently if the user enters a negative timestamp (which ideally shouldn't be the case), say -0.2, then the error message returned is Time point -0.2 is not representable in current resolution. We could possibly have a check for timestamps being negative, and throw a more appropriate warning message.

This comment has been minimized.

@heplesser

heplesser Aug 28, 2017

Contributor

I decided to combine this with the t==0 check: if t <= 0, the user now gets the following message: "Amplitude can only be changed at strictly positive times (t > 0)."

@heplesser

heplesser Aug 28, 2017

Contributor

I decided to combine this with the t==0 check: if t <= 0, the user now gets the following message: "Amplitude can only be changed at strictly positive times (t > 0)."

Show outdated Hide outdated models/step_current_generator.cpp
@jougs

Other than my tiny comment on indentation the code looks good to me.

Can you please add a short explanation on why we need the allow_offgrid_times flag? What would be a use case for this?

Thanks!

Show outdated Hide outdated models/step_current_generator.h
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 29, 2017

Contributor

@jougs One use case would be that the user uses random numbers for the times at which amplitudes change. These would not most likely not fall onto the grid. As a convenience for the user, NEST can round these times, but it should only do this if explicitly requested to avoid subtle errors otherwise.

Should I put this into the inline documentation, or did you just want an explanation here in the discussion?

Contributor

heplesser commented Aug 29, 2017

@jougs One use case would be that the user uses random numbers for the times at which amplitudes change. These would not most likely not fall onto the grid. As a convenience for the user, NEST can round these times, but it should only do this if explicitly requested to avoid subtle errors otherwise.

Should I put this into the inline documentation, or did you just want an explanation here in the discussion?

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Aug 30, 2017

Contributor

@heplesser: thanks for the explanation. I think it won't hurt if you added something along those lines to the documentation. Thanks for the nice work!

Contributor

jougs commented Aug 30, 2017

@heplesser: thanks for the explanation. I think it won't hurt if you added something along those lines to the documentation. Thanks for the nice work!

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 30, 2017

Contributor

@jougs Done, hope it is ready to merge now!

Contributor

heplesser commented Aug 30, 2017

@jougs Done, hope it is ready to merge now!

@jougs

jougs approved these changes Aug 30, 2017

Wonderful. Many thanks!

@jougs jougs merged commit db7dab4 into nest:master Aug 30, 2017

1 check passed

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

@heplesser heplesser deleted the heplesser:fix-740-step_current_generator branch Sep 4, 2017

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