Skip to content

NaN Class#1178

Merged
isuruf merged 2 commits intosymengine:masterfrom
ShikharJ:NaN
Jan 27, 2017
Merged

NaN Class#1178
isuruf merged 2 commits intosymengine:masterfrom
ShikharJ:NaN

Conversation

@ShikharJ
Copy link
Copy Markdown
Member

@ShikharJ ShikharJ commented Jan 20, 2017

@isuruf The builds seem too be failing even though type_codes.inc has been updated. Please review and suggest improvements.

@ShikharJ ShikharJ changed the title [WIP] NaN Class NaN Class Jan 21, 2017
@ShikharJ
Copy link
Copy Markdown
Member Author

Ping @isuruf.

@@ -1,3 +1,6 @@
#ifdef NAN
#undef NAN
#endif
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could create problems for other software including this header. I'd prefer if the typecode was changed.

RCP<const Basic> a;

a = Nan;
REQUIRE(a->__str__() == "nan");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just do Nan->__str__() == "nan"

void bvisit(const NaN &x)
{
std::ostringstream s;
s << "NOT_A_NUMBER";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Make this NAN

extern SYMENGINE_EXPORT RCP<const Infty> ComplexInf;

// Not a Number
extern SYMENGINE_EXPORT RCP<const NaN> Nan;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you change this to just nan?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nan is a builtin as well. Sorry about that. Can you change this back to Nan?

throw SymEngineException("Indeterminate Expression: `Infty +- "
"Infty` encountered. Directions don't "
"match");
} else if (is_unsigned_infinity()) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This condition is not added below

//! NaN is mathematically not equal to anything else, even NaN itself.
bool NaN::__eq__(const Basic &o) const
{
return false;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why this? SymPy gives true for nan == nan

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@isuruf This is from SymPy's documentation:

>>> Eq(nan, nan)   # mathematical equality
    False
 >>> nan == nan     # structural equality
    True

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then we should do the same as __eq__ is structural equality.


int NaN::compare(const Basic &o) const
{
if (is_same_type(*this, o))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See

int Infty::compare(const Basic &o) const
{
SYMENGINE_ASSERT(is_a<Infty>(o))
const Infty &s = down_cast<const Infty &>(o);
return _direction->compare(*(s.get_direction()));
}
for how to implement this.

@ShikharJ ShikharJ force-pushed the NaN branch 3 times, most recently from 54a30aa to 4efe858 Compare January 23, 2017 13:07
@ShikharJ
Copy link
Copy Markdown
Member Author

ShikharJ commented Jan 23, 2017

@isuruf Can you please review this?

REQUIRE(eq(*r1, *r2));

r1 = div(zero, zero);
REQUIRE(r1->__str__() == "nan");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use eq(*r1, *Nan) here and below

Copy link
Copy Markdown
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

Looks good.
Can you resolve conflicts?

n1 = b->div(*c);
REQUIRE(n1->__str__() == "nan");
n1 = c->div(*c);
REQUIRE(n1->__str__() == "nan");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use eq(*n1, *Nan)

inline bool is_complex() const
{
return false;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add is_exact() here and return false.

@ShikharJ ShikharJ force-pushed the NaN branch 3 times, most recently from 08677f7 to 5181bfe Compare January 25, 2017 08:04
@ShikharJ
Copy link
Copy Markdown
Member Author

Ping @isuruf.

@ShikharJ
Copy link
Copy Markdown
Member Author

Ping @isuruf.

@isuruf isuruf merged commit 4f8ce8e into symengine:master Jan 27, 2017
@isuruf
Copy link
Copy Markdown
Member

isuruf commented Jan 27, 2017

Thanks

@ShikharJ ShikharJ deleted the NaN branch January 27, 2017 05:09
ranjithkumar007 pushed a commit to ranjithkumar007/symengine that referenced this pull request Jan 31, 2017
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.

2 participants