-
Notifications
You must be signed in to change notification settings - Fork 37
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
Do you like sets? I like sets! Full sets Module! #585
base: master
Are you sure you want to change the base?
Conversation
ad926a8
to
7e4f112
Compare
Wow! This is pretty cool. Pytave might make at least some of this possible without us writing code (by exposing things like |
Hi, all updated, but i was trying to check this in travis but isn't working D: |
(travis don't is checking the pr) |
looks like it has run now (?) |
071448d
to
9817c11
Compare
Yay!, all updated and working! |
Hi, is possible start merging this? Thx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I started reading through this. A detailed review for correctness will take quite some time!
%% @documentencoding UTF-8 | ||
%% @defmethod @@sym ainterval (@var{x}) | ||
%% Return the union of intervals of x when, @var{x} is in rectangular form | ||
%% or the union of intervals of r when self is in polar form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First line need to fit on one line (its used for lookfor
).
%!test | ||
%! a = interval (sym (0), 1); | ||
%! b = interval (sym (0), 1, true, false); | ||
%! assert( isequal( boundary (a), boundary (b))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I've been very sloppy in the past, but for new code, let's try to use core Octave style (except for sym()
).
|
||
|
||
%% This function is tested in @sym/ainterval, @sym/binterval | ||
%% @sym/ispolar, @sym/psets, @sym/sets and @sym/normalizethetaset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add an %! assert true
or something here too so automated checks don't flag this as untested.
%% @documentencoding UTF-8 | ||
%% @defmethod @@sym complexregion (@var{x}, @var{y}) | ||
%% Represents the Set of all Complex Numbers. | ||
%% It can represent a region of Complex Plane in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line between the "lookfor" and the rest of the body. But this looks wrong? Should it represent all complex numbers if no arguments are passed or what does the lookfor
summary line mean?
|
||
%% -*- texinfo -*- | ||
%% @documentencoding UTF-8 | ||
%% @defmethod @@sym complexregion (@var{x}, @var{y}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
y
should be called something else like @var{polar}
. And please list both ways of calling it:
@defmethod @@sym complexregion (@var{x})
@defmethodx @@sym complexregion (@var{x}, @var{polar})
%% -*- texinfo -*- | ||
%% @documentencoding UTF-8 | ||
%% @defmethod @@sym infimum (@var{x}) | ||
%% The infimum of @var{x}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The infimum of the set @var{x}.
%% -*- texinfo -*- | ||
%% @documentencoding UTF-8 | ||
%% @defmethod @@sym iscomplement (@var{x}) | ||
%% Retrurn True if @var{x} is complement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling mistake and this lookfor line doesn't tell me any more than the name iscomplement
already does. At least put the word "set" in there somewhere.
%% @result{} ans = (sym) True | ||
%% @end group | ||
%% @end example | ||
%% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some/most of these methods need @seealso
%% -*- texinfo -*- | ||
%% @documentencoding UTF-8 | ||
%% @defmethod @@sym isdisjoint (@var{x}, @var{y}) | ||
%% Returns True if @var{x} and @var{y} are disjoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure the word "set" appears in most of these "lookfor" summary lines,
%% -*- texinfo -*- | ||
%% @documentencoding UTF-8 | ||
%% @defmethod @@sym isleftunbounded (@var{x}) | ||
%% Return True if the left endpoint is negative infinity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"return True" is probably not needed (I forgot exactly, you could check the core Octave guidelines on this sort of thing). Use the word "Interval" somewhere.
My biggest concern about all this is whether we should just wait until (and if?) That would keeps things as close to sympy as possible, keeping things slim. OTOH, this is here now and looks like it will be good. I am undecided what to do. Which is probably why I've let this sit so long... |
Hi @cbm755 this time i bring the full sets module (i skip some redundant functions), because we add intervals function with some things but is very incomplete, when i need works with it, it feels... without tools to handle it.
So i think with this we will can works properly with sets.
Obvs, the function names don't are absolutes, if you like we can discuss names for it, in the most of functions i use the sympy name, in others i change it to conserve the octave/matlab prefix.
And this time i take time to check every format!
This pr is required for improve solveset correctly.
Here are two special functions it not belongs directly from sympy sets:
Obvs you can like split this heavy pr, but i think before it select ready things and that type of things.
I think here no are unnecessary functions, things with which we can work.
The original issue: #575
have the original names of sympy functions and some data of this work.
i don't know if all this functions are available in the travis sympy, i'll check it.
As a note, sadly you can't call sets from sym, why?, in a example, to theoretically you need call like this:
but sadly for the
S
function don't like""
in the expression with sets:but if you remove the quotes you will can't make symbols. other point its it will be to confuse to the user, call a lot of possibles things from sym, seems a bad idea, i think its more clear a new function to handle it.
I decide use the S.tuple instead of Point as point for two reasons, first the point function have this issue: sympy/sympy#11683, and in the sets doc the tuple is used to represent point in sets, so it should be safe for now (i write in the doc what happen when you plus two points).
Thx. Cya.