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

example modified #12240

Merged
merged 1 commit into from Mar 3, 2017
Merged

example modified #12240

merged 1 commit into from Mar 3, 2017

Conversation

Yathartha22
Copy link
Member

In bivariate.py function named _mostfunc has an example that is repeated, therefore I tried to modify the example so that one finds it easier to understand what the function does. Following is what I modified:

>>> _mostfunc(exp(x) + exp(exp(y) + 2), exp)
exp(exp(y) + 2)

This example would help one understand that the result also depends on the variable specified, as, if variable(3rd param) is defined it would work accordingly as given in the docs example:

>>> _mostfunc(exp(x) + exp(exp(y) + 2), exp, x)
exp(x)

@kshitij10496
Copy link
Member

@Yathartha22 You have made a good point here.
Can you add this note on the importance of the variable (namely the parameter X) in the docstrings of _most_func for future references?

@Yathartha22
Copy link
Member Author

Surely I would do it right away.

@Yathartha22
Copy link
Member Author

@kshitij10496 I have added the docstring. Is it enough. Please check and let me know.

``X`` has to be a varaible, by default which is `None`. If ``X`` is not `None`
only those terms will be selected that contains the variable specified
as ``X`` and the term returned would be one of these.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the statement can be simplified to:

If X is not None, then the function returns the term composed with the most func

Copy link
Member Author

Choose a reason for hiding this comment

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

If X is not None, then the function returns the term composed with the most func having the specified variable

I think having the specified variable should also be added because even if X is None it would return the term composed with the most func.
what do you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah ! You are correct.
Kindly modify accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay :)

@@ -47,6 +47,10 @@ def _mostfunc(lhs, func, X=None):
``func`` can be a function (exp, log, etc...) or any other SymPy object,
like Pow.

``X`` has to be a varaible, by default which is `None`. If ``X`` is not `None`
Copy link
Member

Choose a reason for hiding this comment

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

Typo: varaible -> variable

@smichr
Copy link
Member

smichr commented Mar 2, 2017

+1

@Yathartha22
Copy link
Member Author

I guess I have made the necessary changes.

@kshitij10496
Copy link
Member

+1
It would be great if you can squash these commits into a single commit.
If you don't know about rewriting history in Git, then I would suggest you use it in your next PR.

@Yathartha22
Copy link
Member Author

I actually don't know how to squash but I will try to google it and find out.

@Yathartha22
Copy link
Member Author

I found this:
$ git log to check for all the commits
$ git rebase -i <parent commit id>
It opens the editor showing commits; apart from the first commit replace pick to s for other commits.
It again opens the editor, put the new commit message and close editor.
$ git push -f to push the commit.

Is this the right procedure?

@ranjithkumar007
Copy link
Contributor

yes, this is a right approach @Yathartha22 .

@Yathartha22 Yathartha22 force-pushed the mostfunc-check branch 2 times, most recently from c7148dc to c1cfab6 Compare March 2, 2017 20:17
@Yathartha22
Copy link
Member Author

thanks @ranjithkumar007

@kshitij10496 I squashed the commits into one. Now, is it okay?

@kshitij10496
Copy link
Member

Awesome @Yathartha22 ! Way to go.

++1
@smichr Can we merge this now ?

@Yathartha22
Copy link
Member Author

Yathartha22 commented Mar 3, 2017

thanks @kshitij10496 :)
Also I have a question that do I have to squash every time I make more than 1 commit in a PR?

@ranjithkumar007
Copy link
Contributor

@Yathartha22 , Its not the case that every time you need to squash your commits in a PR..Squashing is done to have a clear and concise git history that easily documents the changes.this in turn increases the readability of the history.So you need to do this only when the log seems to be mess.
I would also recommend to use a better commit message than rebased2 so that anyone can get a rough idea of what you have done just by reading the message.

@kshitij10496
Copy link
Member

Thanks @ranjithkumar007
I couldn't have said it better than you did.

@Yathartha22 As @ranjithkumar007 suggested, update your commit message accordingly.

@Yathartha22
Copy link
Member Author

thanks @ranjithkumar007 for the explanation and suggestion
I have made changes accordingly, have a look.

@ranjithkumar007
Copy link
Contributor

+1 , looks good to me .

@smichr smichr merged commit 6e50f08 into sympy:master Mar 3, 2017
@smichr
Copy link
Member

smichr commented Mar 3, 2017

Thanks! It's in.

@Yathartha22
Copy link
Member Author

Thanks @smichr for the merge
and also @kshitij10496 and @ranjithkumar007 for guidance and help. I really appreciate :)

@Yathartha22 Yathartha22 deleted the mostfunc-check branch March 3, 2017 14:28
skirpichev pushed a commit to skirpichev/diofant that referenced this pull request Feb 23, 2019
// edited by skirpichev

Based on sympy/sympy#12240

Signed-off-by: Sergey B Kirpichev <skirpichev@gmail.com>
skirpichev pushed a commit to skirpichev/diofant that referenced this pull request Feb 24, 2019
// edited by skirpichev

Based on sympy/sympy#12240

Signed-off-by: Sergey B Kirpichev <skirpichev@gmail.com>
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