Skip to content

Conversation

@ashutoshsaboo
Copy link
Contributor

Issue #66 resolved. Now, the equation listed in #66 - ((sin(x) - cos(x))(sin(x) + cos(x))) == (1 - 2 (cos(x)**2)) returns TRUE.

Same can be observed in the screenshot attached below-:

selection_010

@asmeurer Please review this PR.

Thanks! 😄

@asmeurer
Copy link
Member

This doesn't seem like the right place in the code to implement this. It should be implemented as a card.

@ashutoshsaboo
Copy link
Contributor Author

@asmeurer Sorry, I didn't get your point. Any reason why? Because, it's just string manipulation, I am doing it here, since anywhere else in the cards, I am not able to get the user string input, and I get that only as parsed input(which is broken into different alphabets), and stored in some kind of alphabets.

That's the reason why I implemented it here. @asmeurer

@asmeurer
Copy link
Member

String manipulation should be done at the parser level. I think there might even already be some functions in the parsing module for detecting equalities.

@ashutoshsaboo
Copy link
Contributor Author

Ohh I see. I'll try to have a look at them @asmeurer .

BTW, Why do we generally categorize, like this needs to be done at the parser level? Is it just, so that the code remains organized, or is there any other reason? Since, I didn't know, so thought, I must ask.

Thanks 😄

@asmeurer
Copy link
Member

Yes, it keeps the code organized. Also, the parser is a better abstraction for dealing with strings and converting them to SymPy input. Searching for substrings (like you are doing here) gets messy fast, and more importantly, won't always work in general, because you won't be able to handle things like matching parentheses or detecting when there is a string literal in the string itself. For instance, if someone wrote Symbol('=='), which is completely valid.

@ashutoshsaboo
Copy link
Contributor Author

@asmeurer Ohh yes I understand. I'll look into it, further deep, and try to push a new PR for this issue. Thanks for your input Sir. 😄

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