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.

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

Added the function mpsmpo.mpo_to_pmps() for rank-1 MPOs #52

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

Conversation

MoritzLange
Copy link

Said function was added in collaboration with Milan. It is used in the tests in my py-tedopa program. Apart from that only two checks in mpsmpo.py were improved by adding error messages (also in collaboration with Milan). I was unfortunately not able to write a test for the mpo_to_pmps().

Moritz Lange added 3 commits December 15, 2017 14:49
according to the discussion on the last commit.
according to the discussion on the last commit.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 93.514% when pulling c78ece8 on MoritzLange:master into ae825b2 on dseuss:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 93.514% when pulling c78ece8 on MoritzLange:master into ae825b2 on dseuss:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 93.514% when pulling c78ece8 on MoritzLange:master into ae825b2 on dseuss:master.

@dsuess
Copy link
Owner

dsuess commented Apr 15, 2018

Anything new here?

@MoritzLange
Copy link
Author

I'm in contact with Milan and this is being worked on (slowly but steadily :) ). The current state: I'm afraid I'm unable to write the test for this (due to a mixture of lack of time and lack of experience), but this should be a useful extension of your code, so I decided to post a pull request anyway. About the failing Travis CI build, that seems to have nothing to do with the changes I made (together with Milan). I'm a bit unsure about that. The build for Python 3.5 is failing, in particular one test about it. It throws an error about mpnum.factory.random_mpa, which I didn't even touch.

@dsuess
Copy link
Owner

dsuess commented Apr 24, 2018

Don't worry about the failing tests. I'll have a look.

Regarding tests: What do you think about testing whether mpo -> pmps -> mpo and pmps -> mpo -> pmps are (almost) identities? If you would like to tackle this please let me know.

@MoritzLange
Copy link
Author

MoritzLange commented May 9, 2018

I have thought of testing mpo -> pmps -> mpo and pmps -> mpo -> pmps too, but then looked at your other tests and came to the conclusion that you would not like a test which relies on the correct functioning of another function (pmps_to_mpo() in this case). But now that you've suggested it I've tried to implement that test (sorry for the 2 week delay though). It fails because the normdists are apparently pretty much random and can become as high as 257. Assuming the function to be tested, mpo_to_pmps(), works correctly (it looked like it does when I used it), do you maybe have an idea what the problem with this test is?

@dsuess
Copy link
Owner

dsuess commented May 9, 2018

No worries, looks good so far.

I think we can't really get around tests depending on other parts working correctly. As long as our test-dependency is not circular, I don't mind. The alternative would be to check local tensors by hand, which is much more prone to error.

I'll have a look at the test.

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.

None yet

4 participants