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

Coupled and uncoupled states and operators #524

Merged
merged 9 commits into from Aug 26, 2011

Conversation

Projects
None yet
5 participants
@flacjacket
Member

flacjacket commented Aug 2, 2011

Implements coupled and uncoupled states, including logic to rewrite and represent these states and pass from one to the other. Coupling is done by passing j1 and j2 parameters to the represent or rewrite functions. Uncoupled states are taken as TensorProduct's of normal states.

Implements the logic for uncoupled operators and how operators, both coupled and uncoupled, act on the new states. Unocupled operators, like the uncoupled states, are defined by taking TensorProduct's of normal operators. These operators are able to then act on uncoupled states.

In addition, rewrite logic is changed to allow for symbolic states, both for symbolic j and for when the coupled parameters, j1 and j2, are symbolic.

One thing to note is for all the implemented spin space coupling, it is assumed there are only two coupled spin spaces, functions are not generalizable to higher spin spaces. The code to move up to coupling more spin spaces will be in a later pull.

Fixes:
http://code.google.com/p/sympy/issues/detail?id=2614

TODO:

@ellisonbg

View changes

Show outdated Hide outdated sympy/physics/quantum/represent.py
@ellisonbg

View changes

Show outdated Hide outdated sympy/physics/quantum/spin.py
@@ -123,6 +123,15 @@ class TensorProduct(Expr):
def _eval_dagger(self):
return TensorProduct(*[Dagger(i) for i in self.args])
def _eval_rewrite(self, pattern, rule, **hints):

This comment has been minimized.

@ellisonbg

ellisonbg Aug 3, 2011

Member

The base TensorProduct implementation should not know anything about spin. This logic needs to be moved elsewhere.

@ellisonbg

ellisonbg Aug 3, 2011

Member

The base TensorProduct implementation should not know anything about spin. This logic needs to be moved elsewhere.

@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Aug 3, 2011

Member

Some notes on this branch:

  • The logic in the SpinOpBase.apply_operator* methods has somehow gotten
    turned inside out. These methods all call _apply_op, which seem to handle
    cases where the ket is a non-Ket (Add,Mul,Sum). The apply_operator*
    methods are called by qapply and should never be passed anything than an
    instance of the class that matches the name of the method. Thus,
    _apply_operator_JzKet, must always be passed a JzKet instance. If these
    methods are getting passed anything different it is a bug in qapply
    and should be fixed. The logic here is also horribly circular. _apply_operator
    calls the _apply_operator_Foo methods, which call _apply_op, which calls
    _apply_operator.
  • The basis and basis_opts class attributes are being used by _apply_op, when
    the ket is rewritten. It is not clear why this rewriting is done and
    especially why some operators have coupled:True in basis_opts, but others
    don't.
  • It is also not clear when _apply_op is actually called as many subclasses
    override the _apply_operator_Foo methods.
  • We need to think about how to get specialized spin logic out of represent,
    TensorProduct and qapply.
  • I would like to better understand the logic in _rewrite_basis and the
    overall model of how spin coupling is being handled.

It would probably be best to have an IRC session about this. Is @certik around this week?

Member

ellisonbg commented Aug 3, 2011

Some notes on this branch:

  • The logic in the SpinOpBase.apply_operator* methods has somehow gotten
    turned inside out. These methods all call _apply_op, which seem to handle
    cases where the ket is a non-Ket (Add,Mul,Sum). The apply_operator*
    methods are called by qapply and should never be passed anything than an
    instance of the class that matches the name of the method. Thus,
    _apply_operator_JzKet, must always be passed a JzKet instance. If these
    methods are getting passed anything different it is a bug in qapply
    and should be fixed. The logic here is also horribly circular. _apply_operator
    calls the _apply_operator_Foo methods, which call _apply_op, which calls
    _apply_operator.
  • The basis and basis_opts class attributes are being used by _apply_op, when
    the ket is rewritten. It is not clear why this rewriting is done and
    especially why some operators have coupled:True in basis_opts, but others
    don't.
  • It is also not clear when _apply_op is actually called as many subclasses
    override the _apply_operator_Foo methods.
  • We need to think about how to get specialized spin logic out of represent,
    TensorProduct and qapply.
  • I would like to better understand the logic in _rewrite_basis and the
    overall model of how spin coupling is being handled.

It would probably be best to have an IRC session about this. Is @certik around this week?

@flacjacket

This comment has been minimized.

Show comment
Hide comment
@flacjacket

flacjacket Aug 3, 2011

Member

In response to some points you made:

  • I pulled 'basis_opts', it was hanging around from when states could be defined as coupled, and given the current spin implementation isn't needed
  • I changed _apply_op to do less reinventing of the qapply wheel. The reason that was in place was because there is a rewrite step to get the state into a basis that the operator can act on the state, which could lead to a linear combination of states. Really, all apply_op does is write the state in a basis where the action of the operator on the state is known (i.e. a basis such that _apply_operator_Foo is overridden by the operator in question). There was some additional logic in _apply_op that was meant to deal with when there is a Sum returned by the rewrite for a symbolic state, but that could be something to do in qapply.
    I tried to push the coupling logic as far down as it made sense to do so without changing how things are currently implemented, but that and your other last couple points would probably be better discussed on IRC.
Member

flacjacket commented Aug 3, 2011

In response to some points you made:

  • I pulled 'basis_opts', it was hanging around from when states could be defined as coupled, and given the current spin implementation isn't needed
  • I changed _apply_op to do less reinventing of the qapply wheel. The reason that was in place was because there is a rewrite step to get the state into a basis that the operator can act on the state, which could lead to a linear combination of states. Really, all apply_op does is write the state in a basis where the action of the operator on the state is known (i.e. a basis such that _apply_operator_Foo is overridden by the operator in question). There was some additional logic in _apply_op that was meant to deal with when there is a Sum returned by the rewrite for a symbolic state, but that could be something to do in qapply.
    I tried to push the coupling logic as far down as it made sense to do so without changing how things are currently implemented, but that and your other last couple points would probably be better discussed on IRC.
@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Aug 11, 2011

Member

Test results html report: http://pastehtml.com/view/b3im95u8t.html

Summary: There do not appear to be any problems.

Please double check this against the test report given above before merging with master.

Automatic review by sympy-bot.

Member

asmeurer commented Aug 11, 2011

Test results html report: http://pastehtml.com/view/b3im95u8t.html

Summary: There do not appear to be any problems.

Please double check this against the test report given above before merging with master.

Automatic review by sympy-bot.

@flacjacket

This comment has been minimized.

Show comment
Hide comment
@flacjacket

flacjacket Aug 15, 2011

Member

I have addressed all the things we discussed and was in the email. One thing, tho, is there are now some opperatorset tests that fail due to the new method of the SpinState class explicitly taking a j and m parameter, where the tests invoke it without any parameters. Maybe it's something @lazovich can comment on.

Member

flacjacket commented Aug 15, 2011

I have addressed all the things we discussed and was in the email. One thing, tho, is there are now some opperatorset tests that fail due to the new method of the SpinState class explicitly taking a j and m parameter, where the tests invoke it without any parameters. Maybe it's something @lazovich can comment on.

@lazovich

This comment has been minimized.

Show comment
Hide comment
@lazovich

lazovich Aug 15, 2011

Contributor

@flacjacket: This is something that is pretty easily fixed, I think. There's new code that I added in QExpr that will look for a default_args classmethod to try to fill in the args if a state is initialized with no parameters.

So, if you add something like this to SpinState it should solve the problem:

@classmethod
def default_args(self):
     return ("j", "m")

You can see cartesian.py for examples of how I have these in the continuous state classes.
You can also make those args Symbols explicitly if you'd like them to have certain assumptions with them. Otherwise, they'll just be sympified when you initialize the state.

If you don't want to add the default_args method, you should be able to just change the tests to invoke the state with explicit j and m arguments.

Hope that helps. Let me know if you have more questions!

Contributor

lazovich commented Aug 15, 2011

@flacjacket: This is something that is pretty easily fixed, I think. There's new code that I added in QExpr that will look for a default_args classmethod to try to fill in the args if a state is initialized with no parameters.

So, if you add something like this to SpinState it should solve the problem:

@classmethod
def default_args(self):
     return ("j", "m")

You can see cartesian.py for examples of how I have these in the continuous state classes.
You can also make those args Symbols explicitly if you'd like them to have certain assumptions with them. Otherwise, they'll just be sympified when you initialize the state.

If you don't want to add the default_args method, you should be able to just change the tests to invoke the state with explicit j and m arguments.

Hope that helps. Let me know if you have more questions!

@lazovich

This comment has been minimized.

Show comment
Hide comment
@lazovich

lazovich Aug 15, 2011

Contributor

@flacjacket: Actually, scratch that. I don't think the default_args solution will work because you override QExpr's new. The easiest solution is probably to just change the operatorset tests to pass an explicit j and m.

Contributor

lazovich commented Aug 15, 2011

@flacjacket: Actually, scratch that. I don't think the default_args solution will work because you override QExpr's new. The easiest solution is probably to just change the operatorset tests to pass an explicit j and m.

@lazovich

This comment has been minimized.

Show comment
Hide comment
@lazovich

lazovich Aug 16, 2011

Contributor

@flacjacket: Ah forgive the spam but as I think about this more I realize that the problem is actually a bit more subtle. Let me explain: operatorset.py has a dictionary which contains eigenstate to operator mappings, and vice versa. The whole point of this is to be able to call represent with basis=SomeOperatororSet or basis=SomeEigenket and have them do the same thing. The mapping functions in operatorset always try to return an instance of the thing they're mapping to, in this case a state. Here, the tests are expecting a "default" instance. The new in QExpr tries to call, as I mentioned before, a default_args method in the state if no parameters are passed. Since you're overriding new, one solution would be to add the default_args method as I suggested, and then change your new to check for it if no arguments are passed. (Rather than taking the j and m arguments explicitly in new, as you do now, you could just change the parameters to *args and then check the length).

Alternatively, since this doesn't directly affect your code for the time being, the easiest solution is probably to just comment out or delete the failing spin tests in operatorset and get this merged in. I'll then rebase on top of it and see if I can get default instances working in SpinState. (I have some other cosmetic changes to spin classes in my represent2 branch anyway, so this might be the best way to go).

Contributor

lazovich commented Aug 16, 2011

@flacjacket: Ah forgive the spam but as I think about this more I realize that the problem is actually a bit more subtle. Let me explain: operatorset.py has a dictionary which contains eigenstate to operator mappings, and vice versa. The whole point of this is to be able to call represent with basis=SomeOperatororSet or basis=SomeEigenket and have them do the same thing. The mapping functions in operatorset always try to return an instance of the thing they're mapping to, in this case a state. Here, the tests are expecting a "default" instance. The new in QExpr tries to call, as I mentioned before, a default_args method in the state if no parameters are passed. Since you're overriding new, one solution would be to add the default_args method as I suggested, and then change your new to check for it if no arguments are passed. (Rather than taking the j and m arguments explicitly in new, as you do now, you could just change the parameters to *args and then check the length).

Alternatively, since this doesn't directly affect your code for the time being, the easiest solution is probably to just comment out or delete the failing spin tests in operatorset and get this merged in. I'll then rebase on top of it and see if I can get default instances working in SpinState. (I have some other cosmetic changes to spin classes in my represent2 branch anyway, so this might be the best way to go).

@flacjacket

This comment has been minimized.

Show comment
Hide comment
@flacjacket

flacjacket Aug 16, 2011

Member

Thanks for the info Tomo, I've commented out the tests and now everything passes fine. If you need any help with the spin stuff, let me know. Any other comments or review would be great.

Member

flacjacket commented Aug 16, 2011

Thanks for the info Tomo, I've commented out the tests and now everything passes fine. If you need any help with the spin stuff, let me know. Any other comments or review would be great.

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Aug 16, 2011

Member

@flacjacket, don't comment out tests. Use the XFAIL decorator.

Put the assert statements that you have commented out in a new function, and put @XFAIL above the function definition. You will need to import XFAIL from sympy.utilities.pytest. This way, the test runner will still try to run them, but will not bug you if they fail (XFAIL stands for "expected to fail"), but it will notify you if they start to pass.

Member

asmeurer commented Aug 16, 2011

@flacjacket, don't comment out tests. Use the XFAIL decorator.

Put the assert statements that you have commented out in a new function, and put @XFAIL above the function definition. You will need to import XFAIL from sympy.utilities.pytest. This way, the test runner will still try to run them, but will not bug you if they fail (XFAIL stands for "expected to fail"), but it will notify you if they start to pass.

@flacjacket

This comment has been minimized.

Show comment
Hide comment
@flacjacket

flacjacket Aug 16, 2011

Member

Thanks @asmeurer for noting that, the tests are now in an XFAIL

Member

flacjacket commented Aug 16, 2011

Thanks @asmeurer for noting that, the tests are now in an XFAIL

@certik

This comment has been minimized.

Show comment
Hide comment
@certik

certik Aug 25, 2011

Member

Test results html report: http://pastehtml.com/view/b51wt1xf9.html

Summary: There were test failures.

@flacjacket: Please fix the test failures.

Automatic review by sympy-bot.

Member

certik commented Aug 25, 2011

Test results html report: http://pastehtml.com/view/b51wt1xf9.html

Summary: There were test failures.

@flacjacket: Please fix the test failures.

Automatic review by sympy-bot.

@certik

This comment has been minimized.

Show comment
Hide comment
@certik

certik Aug 25, 2011

Member

Looking at the failures, it seems to me, that it's just 2**(1/2) -> sqrt(2) change. Was this some recent change in sympy?

Member

certik commented Aug 25, 2011

Looking at the failures, it seems to me, that it's just 2**(1/2) -> sqrt(2) change. Was this some recent change in sympy?

@lazovich

This comment has been minimized.

Show comment
Hide comment
@lazovich

lazovich Aug 25, 2011

Contributor

Yes there was a recent change in the printing of sqrt. This affected my branch as well.

Contributor

lazovich commented Aug 25, 2011

Yes there was a recent change in the printing of sqrt. This affected my branch as well.

@flacjacket

This comment has been minimized.

Show comment
Hide comment
@flacjacket

flacjacket Aug 25, 2011

Member

Okay, I'll fix that when I get back from classes.

Member

flacjacket commented Aug 25, 2011

Okay, I'll fix that when I get back from classes.

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer Aug 25, 2011

Member

@certik, yes, see my blog. This was a necessary and long awaited (by me) change, but unfortunately it is going to break half of the open pull requests. But they are trivial to fix, though, so no sweat.

Member

asmeurer commented Aug 25, 2011

@certik, yes, see my blog. This was a necessary and long awaited (by me) change, but unfortunately it is going to break half of the open pull requests. But they are trivial to fix, though, so no sweat.

flacjacket added some commits Jun 27, 2011

Coupled and uncoupled states and operators
Implements coupled and uncoupled states, including logic to rewrite and
represent these states and pass from one to the other through the
rewrite and represent functions. Also improves the handling of symbolic
states for rewriting. Coupled states are specified by taking normal
states and passing j1,j2 parameters to the represent or rewrite
functions. Uncoupled states are taken as TensorProduct's of normal
states.

Implements the logic for uncoupled operators and how operators, both
coupled and uncoupled, act on the new states. Unocupled operators, like
the uncoupled states, are defined by taking TensorProduct's of normal
operators. These operators are able to then act on uncoupled states.

For all the implemented spin space coupling, it is assumed there are
only two coupled spin spaces, functions are not generalizable to higher
spin spaces.
@flacjacket

This comment has been minimized.

Show comment
Hide comment
@flacjacket

flacjacket Aug 26, 2011

Member

This branch should now pass all the tests.

Member

flacjacket commented Aug 26, 2011

This branch should now pass all the tests.

@certik

This comment has been minimized.

Show comment
Hide comment
@certik

certik Aug 26, 2011

Member

Test results html report: http://pastehtml.com/view/b54s54ok5.html

Summary: There do not appear to be any problems.

Please double check this against the test report given above before merging with master.

Automatic review by sympy-bot.

Member

certik commented Aug 26, 2011

Test results html report: http://pastehtml.com/view/b54s54ok5.html

Summary: There do not appear to be any problems.

Please double check this against the test report given above before merging with master.

Automatic review by sympy-bot.

@certik

This comment has been minimized.

Show comment
Hide comment
@certik

certik Aug 26, 2011

Member

Looks good to me, merging.

Member

certik commented Aug 26, 2011

Looks good to me, merging.

certik added a commit that referenced this pull request Aug 26, 2011

Merge pull request #524 from flacjacket/coupled_spin
Coupled and uncoupled states and operators

@certik certik merged commit 1e90da5 into sympy:master Aug 26, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment