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

Minor refactoring in Time as discussed in PR #685 #706

Merged
merged 1 commit into from
Jun 29, 2017

Conversation

jakobj
Copy link
Contributor

@jakobj jakobj commented Apr 12, 2017

This PR implements the changes proposed during review of #685.
I suggest @heplesser @apeyser or @gtrensch as reviewers

@heplesser heplesser added ZC: Kernel DO NOT USE THIS LABEL I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) ZP: PR Created DO NOT USE THIS LABEL S: High Should be handled next T: Bug Wrong statements in the code or documentation T: Enhancement New functionality, model or documentation and removed T: Bug Wrong statements in the code or documentation labels Apr 18, 2017
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakobj Thanks! I added a suggestion which I believe will make the code even more readable.

@@ -492,11 +492,11 @@ class Time
double
get_ms() const
{
if ( tics == LIM_POS_INF.tics )
if ( tics >= LIM_POS_INF.tics )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this change is based on a suggestion I made, but I think we can do better here. I checked the Time class once more and we have checks in place there that ensure that LIM_NEG_INF.tics <= tics <= LIM_POS_INF.tics at all times. We also have a is_neg_inf() method; adding a corresponding is_pos_inf() method, we can then turn this into

if ( is_pos_inf() )
{
  return LIM_POS_INF.tics;
}
if ( is_neg_inf() )
{
  return LIM_NEG_INF.tics;
}

I think this would be even more readable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @heplesser.

In my opinion this calls for a macro and/or a function (e.g. IS_POSINFINITY() preserve_infinity()). But this is beyond the scope of this PR and would require additional changes to the time class to handle the different infinity value checks in a more generic way.

@@ -506,11 +506,11 @@ class Time
delay
get_steps() const
{
if ( tics == LIM_POS_INF.tics )
if ( tics >= LIM_POS_INF.tics )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above.

@heplesser heplesser requested a review from apeyser April 18, 2017 08:30
@jakobj jakobj force-pushed the enh/cleanup_time branch from 153f48d to dfd2811 Compare June 12, 2017 15:24
@jakobj
Copy link
Contributor Author

jakobj commented Jun 12, 2017

Thanks @heplesser for the suggestion. I implemented that now.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just another little detail ...

@@ -198,11 +197,11 @@ Time::reset_to_defaults()

std::ostream& operator<<( std::ostream& strm, const Time& t )
{
if ( t.tics == Time::LIM_NEG_INF.tics )
if ( t.tics <= Time::LIM_NEG_INF.tics )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakobj I know this is getting a bit nit-picky, but since we are at it, I think we should beautify this to

if ( t.is_neg_inf() )

and the same below for the positive infinity case.

@jakobj jakobj force-pushed the enh/cleanup_time branch from dfd2811 to 9b12d58 Compare June 13, 2017 09:07
@jakobj
Copy link
Contributor Author

jakobj commented Jun 13, 2017

@heplesser just fixed the last detail ;)

@heplesser
Copy link
Contributor

@jakobj This looks fine, but I am a bit confused by the fact that there is only a single commit visible in Github for this PR, dated 12 April, even though you have made changes during the review. Did you amend that commit or squash commits?

@@ -387,7 +387,13 @@ class Time
bool
is_neg_inf() const
{
return tics == LIM_NEG_INF.tics;
return tics <= LIM_NEG_INF.tics;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be called after computations that stay in range, but go beyond the limit? I ask to check -- why should tics ever be legally < LIM_NEG_INF or > LIM_POS_INF?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apeyser As far as I can see from the nest_time.h code, tics can never become smaller than LIM_NEG_INF.tics; the only way for this to happen would be a Time method calling the protected Time(tic_t) constructor. I still had suggested the change above from the perspective of defensive programming: if LIM_NEG_INF.tics represent negative infinity, any smaller value cannot be larger and thus must be infinity as well.

On the other hand, one could argue that LIM_NEG_INF.tics is a "magic value" (just as Inf for doubles is represented by a specific bit pattern), and that we should match that exactly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heplesser so, the choices are: leave as is, replace the inequality portion with an assertion, remove the inequality.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apeyser Correct, where

  • "leave as is" means <=,
  • "remove inequality" means ==,
  • and "replace the inequality portion" means assert(tics >= ...); return ... == ...;
    I would want to avoid the assertion for performance reasons, so maybe we should keep the code as it is currently in master, where it is actually == ( @jakobj Sorry for the back-and-forth!).

Copy link
Contributor

@apeyser apeyser Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heplesser
The assertion should be better performing than leave as is, and should only be worse than remove inequality when the assertion is activated with debug flags. Thus, it would be the best of both worlds. (SIde thing to check -- do we test debug builds in travis and performance builds?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakobj I'd remove the inequality and comment, but if you prefer to leave the inequality and comment, that's fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the inequality. I will add a condensed version of this as a comment.

@jakobj
Copy link
Contributor Author

jakobj commented Jun 14, 2017

@heplesser yes, I squashed all commits since the form one logical unit. this is why only one commit appears.

@heplesser
Copy link
Contributor

I think merging #685 introduced some conflicts.

@jakobj jakobj force-pushed the enh/cleanup_time branch from 9b12d58 to c4d37d9 Compare June 20, 2017 21:30
@jakobj
Copy link
Contributor Author

jakobj commented Jun 20, 2017

Fixed the conflicts and added a comment on the use of <= over ==.

Copy link
Contributor

@gtrensch gtrensch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me!

@heplesser heplesser added this to the NEST 2.12.1 milestone Jun 29, 2017
@jougs
Copy link
Contributor

jougs commented Jun 29, 2017

@apeyser: are the changes OK with you now? If yes, please click the 'approve' button. Thanks!

@apeyser apeyser merged commit 368bee6 into nest:master Jun 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Enhancement New functionality, model or documentation ZC: Kernel DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants