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.

Sign up for GitHub

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

Jump to bottom

Add support for scalar multiplication by Symbol in waves module #27384

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add support for scalar multiplication by Symbol in waves module #27384

wants to merge 4 commits into from

Conversation

RushabhMehta2005
Copy link
Contributor

References to other Issues or PRs

Brief description of what is fixed or changed

This PR implements multiplication operation of a transverse wave (TWave) with a symbolic scalar (Symbol) i.e amplitude scaling by a Symbol.
Example Usage:

>>> A, f, phi= symbols('A, f, phi')
>>> s = symbols('s') # scaling factor
>>> w = TWave(A, f, phi)
>>> w2 = w*s
>>> w2
TWave(A*s, f, phi, 1/f, n)

Tests have also been added.

Other comments

Any feedback and suggestions on improving/enhancing this implementation are appreciated.

Release Notes

  • physics.optics
    • Symbolic amplitude scaling of waves is now supported.

@sympy-bot
Copy link

✅

Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.14.

Click here to see the pull request description that was parsed. 
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed
This PR implements multiplication operation of a transverse wave (TWave) with a symbolic scalar (Symbol) i.e amplitude scaling by a Symbol.
Example Usage:

```python
>>> A, f, phi= symbols('A, f, phi')
>>> s = symbols('s') # scaling factor
>>> w = TWave(A, f, phi)
>>> w2 = w*s
>>> w2
TWave(A*s, f, phi, 1/f, n)
```


Tests have also been added.

#### Other comments
Any feedback and suggestions on improving/enhancing this implementation are appreciated.

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers. Formerly, `log(-x)` incorrectly gave `-log(x)`.

* physics.units
  * Corrected a semantical error in the conversion between volt and statvolt which
    reported the volt as being larger than the statvolt.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* physics.optics
  * Symbolic amplitude scaling of waves is now supported.
<!-- END RELEASE NOTES -->

@RushabhMehta2005
Copy link
Contributor Author

@haru-44 could you possibly review this?

@oscarbenjamin
Copy link
Collaborator

@oscarlazoarjona would you like to review this?

@haru-44
Copy link
Member

haru-44 commented Dec 24, 2024

I'm sorry, but I'm not well-versed in this field, so I can only point out superficial matters.

First, is the syntax of .mailmap correct?

sympy/.mailmap

Lines 142 to 148 in fec342e

# It is important to make sure that each author is listed only once in the
# AUTHORS file so do *NOT* add separate entries that begin with different names
# or email addresses:
#
# # DO NOT DO THIS (it will be treated as two unrelated authors):
# Joe Bloggs <joe@bloggs.com>
# joeb <12345+joeb@users.noreply.github.com>

Next, you've modified it to allow Symbol in addition to Number, but wouldn't values like $e$ or $\pi$ be allowed as well?

@RushabhMehta2005
Copy link
Contributor Author

The purpose of my pull request is to enable the multiplication of wave objects with instances of Symbol. This functionality can be particularly useful in various scenarios but on the other hand multiplying waves with transcendental constants like euler's number or π is relatively uncommon.

@haru-44
Copy link
Member

haru-44 commented Dec 25, 2024

My question was whether it would be possible to use something like isinstance(other, AtomicExpr) instead of isinstance(other, (Number, Symbol)). If a superclass can be specified, it would be better to do so rather than adding individual classes.

@RushabhMehta2005
Copy link
Contributor Author

RushabhMehta2005 commented Dec 25, 2024 •
edited
Loading

That does make sense. Let me try to implement it.

@RushabhMehta2005
Copy link
Contributor Author

The current implementation already supports floats, rationals and symbols. Checking only if TWave instance is multiplied by instance of AtomicExpr invites the need to seperately handle the cases for pi and E.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
physics.optics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RushabhMehta2005 @sympy-bot @oscarbenjamin @haru-44