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

Fixes Rising Factorial #39409

Closed
wants to merge 4 commits into from
Closed

Conversation

harith-hacky03
Copy link

Fixes Rising Factorial

Fixed Rising Factorial Function Implementation.

Fixes #39406

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@user202729
Copy link
Contributor

Can you add a test for this? (such that the test fails before the change, of course)

@harith-hacky03
Copy link
Author

@user202729 so basically you want me to add the test cases and before and after input and output, am I right buddy?

@vincentmacri
Copy link
Contributor

@user202729 so basically you want me to add the test cases and before and after input and output, am I right buddy?

@user202729 is referring to this from the developer guide:

Bugfixes: If the PR contains a bugfix, does it add a doctest illustrating that the bug has been fixed? This new doctest should contain the issue or PR number, for example See :issue:12345.

@vincentmacri
Copy link
Contributor

vincentmacri commented Jan 30, 2025

* [x]  I have created tests covering the changes.

* [x]  I have updated the documentation and checked the documentation preview.

You didn't add any tests or change the documentation yet. Please do not mark these tasks as being done until you've completed them. And for future contributions, not all of these are relevant for every PR. You only need to check off the items relevant to the PR, and you should not check them off until you've completed them.

Copy link

github-actions bot commented Feb 3, 2025

Documentation preview for this PR (built with commit a7be917; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@DaveWitteMorris
Copy link
Member

I don't think making a change is consistent with the documentation, which says:

    Definition: for integer `a \ge 0` we have
    `x(x+1) \cdots (x+a-1)`. In all other cases we use the
    GAMMA-function: `\frac {\Gamma(x+a)} {\Gamma(x)}`.

I think the current behaviour matches the documentation, so if the behaviour is changed, then I think there needs to be a different explanation of what this function is doing.

@harith-hacky03
Copy link
Author

@vincentmacri @DaveWitteMorris Can you please guide me , what to do now? . Sorry for taking your valuable time , but it means a lot for me. Thank you : ) .

@vincentmacri
Copy link
Contributor

@vincentmacri @DaveWitteMorris Can you please guide me , what to do now? . Sorry for taking your valuable time , but it means a lot for me. Thank you : ) .

Start with looking through https://doc.sagemath.org/html/en/developer/. In particular this: https://doc.sagemath.org/html/en/developer/coding_basics.html#the-docstring-of-a-function-content

You are trying to fix #39406, which reports an instance of mathematically incorrect output involving this function. Bug fixes in Sage must be accompanied by a doctest showing that they are fixed with a link to the fixed issue (see the developer guide section on docstrings that I linked for how to do that). This is what @user202729 asked you to do.

@DaveWitteMorris pointed out that the current docstring for the rising_factorial function says that the gamma formula is used if the input is not an integer. Fixing #39406 requires changing the rising_factorial function to better handle expressions with unknown values. This change would deviate from the current documented behaviour of the rising_factorial function, and so @DaveWitteMorris is asking you to update the documentation accordingly (@DaveWitteMorris correct me if I misunderstood your comment).

@DaveWitteMorris
Copy link
Member

Yes, that is what I meant, but after looking at the issue more closely, I need to change my story. See this comment on the original issue.

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

Successfully merging this pull request may close these issues.

wrong subs in rising_factorial
4 participants