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

Misc fixes #1237

Merged
merged 13 commits into from Apr 29, 2022
Merged

Misc fixes #1237

merged 13 commits into from Apr 29, 2022

Conversation

skirpichev
Copy link
Collaborator

No description provided.

@skirpichev skirpichev added this to the 0.14 milestone Apr 24, 2022
@skirpichev skirpichev added series wrong answer if mathematically wrong result was obtained enhancement new feature requests (or implementation) labels Apr 24, 2022
@skirpichev skirpichev force-pushed the series-dir branch 2 times, most recently from 55dffad to da75078 Compare April 25, 2022 03:15
@skirpichev
Copy link
Collaborator Author

@jksuom, it seems it's you merged sympy/sympy#19555. This cdir interface looks to be redundant for me and the docs aren't helpful. Why not pass the dir parameter to the series helpers instead? (BTW, this PR adds some support for complex directions.) Sorry for disturbing, but I would appreciate your reply.

@skirpichev skirpichev changed the title core: change semantics of the dir kwarg of Expr.series() Misc fixes Apr 27, 2022
@jksuom
Copy link
Contributor

jksuom commented Apr 27, 2022

There were two reasons for introducing cdir.
First, it is sometimes necessary to use non-real directions in order to determine the proper branch of a multivalued functions at a branch point. For example,

In [41]: log(x - 1).as_leading_term(x)                                          
Out[41]: ⅈ⋅π

In [42]: log(x - 1).as_leading_term(x, cdir=-I)                                 
Out[42]: -ⅈ⋅π

It is possible to work around this, of course, by coding the direction in the function like log(-I*x - 1) but using cdir seems more flexible to me.

Secondly, dir would have to be "decoded" in each leading term and series method by something like

if dir == '+':
    ...
elif dir == '-':
    ...
else:
    ...

while cdir can be handled with less code. It seemed more convenient to me to make Limit.doit() translate the string form dir to a number like cdir.

It is true that the documentation is missing details on cdir. I was expecting that the GSoC students that were working with these matters would have added some documentation but it seems that they have not found time to do so yet.

It might be possible to dispense with cdir and make dir a number, but it seemed to me that the name cdir would better indicate its nature as a complex direction.

@skirpichev
Copy link
Collaborator Author

It is possible to work around this, of course, by coding the direction in the function like log(-I*x - 1) but using cdir seems more flexible to me.

This pr adds something like the later approach, when dir stands for the complex direction and there is some preprocessing to transform things to stuff like log(-I*x - 1), as in your example. It seems to be simpler with regard to changing the helpers interfaces (i.e. _eval_nseries() and so on). Do you think this will miss some things?

(dir was changed for the Limit earlier: #1234, #1232 and #1235)

Secondly, dir would have to be "decoded" while cdir can be handled with less code.

That's true, but I don't understand why there are two parameters. Their interaction seems unclear for me.

It is true that the documentation is missing details on cdir. I was expecting that the GSoC students that were working with these matters would have added some documentation but it seems that they have not found time to do so yet.

Yeah, that happens after GSoC...

It might be possible to dispense with cdir and make dir a number, but it seemed to me that the name cdir would better indicate its nature as a complex direction.

Rather, with an expression (like Direction in the Mathematica): actual value to be passed to helpers would be something like sign(dir). Not sure about naming (dir also clash with the builtin).

Comment on lines +951 to +953
return FiniteSet(*(p for p, c in [(self.start, not self.left_open),
(self.end, not self.right_open)]
if abs(p) != oo or c))
Copy link
Contributor

Choose a reason for hiding this comment

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

A boundary point of a set does not have to belong to the set. The boundary of an interval contains the endpoints regardless of the type of the interval. I would also consider infinities as boundary points.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The boundary of an interval contains the endpoints regardless of the type of the interval.

That depends on the topology, right? Are you sure, that in the standard topology on the ℝ=(-oo, oo) it's true? There is no open set that contains oo, for example. Also, oo not in the closure of the (it's a closed set and oo not it's member).

Copy link
Contributor

Choose a reason for hiding this comment

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

oo not in the closure of the ℝ

oo must be in the closure of R. Otherwise it could not be the limit of any sequences of real numbers.

The standard topology on ordered sets like R and R_bar(= [-oo, oo]) is the order topology. Its subbase consists of "half-bounded" intervals like (a, oo] and [-oo, b] in R_bar. In particular, intervals like (a, oo] are open neighborhoods of oo (and always contain elements of R).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, shouldn't be the boundary - a complement of the set interior in its closure? Given R is open and closed - its boundary seems to be empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its boundary is also relative to the topological space. Its boundary is empty in R but nonempty in R_bar. If it is decided that R.is_closed should be relative to R (i.e. True), then its boundary should be empty, I guess... In that case, it looks like -oo and oo should not be boundary points of any interval in R. (But maybe there could be "extended intervals".)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its boundary is empty in R but nonempty in R_bar. If it is decided that R.is_closed should be relative to R (i.e. True), then its boundary should be empty, I guess...

Ok, thanks. Then I will adopt this approach for a while.

@skirpichev skirpichev marked this pull request as ready for review April 28, 2022 03:51
@skirpichev
Copy link
Collaborator Author

skirpichev commented Apr 28, 2022 via email

@jksuom
Copy link
Contributor

jksuom commented Apr 28, 2022

It seems that the attribute is_closed is ambiguous. Its value depends on the total space, R or R_bar. R is closed in itself but not in R_bar. A semi-infinite interval like [a, oo) is closed in R but not in R_bar. Maybe it should be decided that if S is a subset of R, then S.is_closed should mean that it is closed in R. (It looks like that is the current convention.)

In case of is_open there should be no problem as a subset of R is open in R if and only if it is open in R_bar.

@skirpichev
Copy link
Collaborator Author

skirpichev commented Apr 28, 2022 via email

@skirpichev skirpichev merged commit 45006b4 into diofant:master Apr 29, 2022
@skirpichev skirpichev deleted the series-dir branch April 29, 2022 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concrete core enhancement new feature requests (or implementation) integrals maintainability series sets testing wrong answer if mathematically wrong result was obtained
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants