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

bugfix/issue-stan-2433: change step to match int_step #805

Closed
wants to merge 2 commits into from
Closed

bugfix/issue-stan-2433: change step to match int_step #805

wants to merge 2 commits into from

Conversation

roualdes
Copy link
Collaborator

Submission Checklist

  • Run unit tests: ./runTests.py test/unit
  • Run cpplint: make cpplint
  • Declare copyright holder and open-source license: see below

Summary:

Change stan::math::step to match stan::math::int_step as requested in stan-dev/stan issue 2433.

Intended Effect:

Consistency between step functions.

How to Verify:

./runTests.py -j3 test/unit/math/prim/scal/

and

./runTests.py -j3 test/unit/math/rev/scal/

I ran the above commands without error.

Side Effects:

Change is step function behavior.

Documentation:

Made a note about this change just above step function definition.

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): California State University, Chico

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

Copy link
Contributor

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

The code and doc look great. Thanks.

We need to check with @syclik to make sure this change is OK with him. I'll ping him and you via email on this.

@bob-carpenter
Copy link
Contributor

@roualdes and @syclik:

I think I led @roualdes astray with the poorly formulated issue 2433 in stan-dev/math. We are very sloppy in using issues and sometimes just use them to track things that need to be fixed in some way without properly deciding how they should be fixed.

The current situation is bad---we have a step() function and int_step() function which are inconsistent. But if we change the behavior of step(), we break backward compatibility.

Here's the BUGS manual. You'll see that they define step(e) as 1 if e >= 0; 0 otherwise. BUGS followed the tradition of defining the Heaviside step function which has that pesky >= condition.

In retrospect, I think we should just deprecate int_step and keep step() matching BUGS and tradition.

Copy link
Contributor

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

I don't think this should be merged. The code's fine, though.

See my comment in the conversation. So I'll mark this request changes.

@syclik
Copy link
Member

syclik commented May 15, 2018

@bob-carpenter, thanks for looking into it.

@syclik
Copy link
Member

syclik commented May 24, 2018

@bob-carpenter, can you update the issue with what you're thinking about? stan-dev/stan#2433

And then we can close this PR.

@bob-carpenter
Copy link
Contributor

bob-carpenter commented May 24, 2018

Sorry, @roualdes but I'm going to close this and rewrite issue stan-dev/stan#2433

@bob-carpenter
Copy link
Contributor

bob-carpenter commented May 24, 2018 via email

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

Successfully merging this pull request may close these issues.

None yet

4 participants