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

Issue2099 #2111

Merged
merged 11 commits into from Mar 22, 2023
Merged

Issue2099 #2111

merged 11 commits into from Mar 22, 2023

Conversation

lklivingstone
Copy link
Contributor

Help needed in the following

  • Contributions to qutip should follow the pep8 style.

I did the pycodestyle on my code, but it is highlighting errors that were there in the original code. It returned lines that have >linespace errors and more, should I edit them too?
Output:

D:\qutip\qutip\core\cy>pycodestyle --first qobjevo.pyx
qobjevo.pyx:1:1: E265 block comment should start with '# '
qobjevo.pyx:2:80: E501 line too long (83 > 79 characters)
qobjevo.pyx:20:37: E211 whitespace before '('
qobjevo.pyx:46:56: W605 invalid escape sequence '\s'
qobjevo.pyx:190:30: E225 missing whitespace around operator
qobjevo.pyx:423:5: E303 too many blank lines (2)
qobjevo.pyx:426:5: E301 expected 1 blank line, found 0
qobjevo.pyx:451:34: E127 continuation line over-indented for visual indent
qobjevo.pyx:572:21: E128 continuation line under-indented for visual indent
qobjevo.pyx:987:39: E231 missing whitespace after ','
qobjevo.pyx:989:20: E124 closing bracket does not match visual indentation
  • Please add tests to cover your changes if applicable.

The issue was to add __repr__ function to QobjEvo class. Do I need to add a test? If so, can someone help me with it?

  • If the behavior of the code has changed or new feature has been added, please also update the documentation in the doc folder, and the notebook. Feel free to ask if you are not sure.

I am not sure in what bracket this issue falls. The issue was labeled as ENH.

  • Include the changelog in a file named: doc/changes/<PR number>.<type> 'type' can be one of the following: feature, bugfix, doc, removal, misc, or deprecation (see here for more information).

Related to the previous point.

Description
Added __repr__ to QobjEvo

It is showing the following attributes: dims, shape, type, superop (if a super operator is present), isconstant and num_elements.

Output:

>>> import qutip
>>> qutip.QobjEvo([qutip.qeye(2), lambda t: t])
<QobjEvo dims = [[2], [2]], shape= (2, 2), type = oper, superrep = None, isconstant = False, num_elements = 1>

Related issues or PRs
Fix #2099

@AGaliciaMartinez
Copy link
Member

AGaliciaMartinez commented Mar 5, 2023

I did the pycodestyle on my code, but it is highlighting errors that were there in the original code. It returned lines that have >linespace errors and more, should I edit them too?

I am going to leave this up to you. It is not required as long as the lines you modified follow PEP8. Although, fixing the output of pycodestyle in the same file would be very much welcome 😄.

The issue was to add repr function to QobjEvo class. Do I need to add a test? If so, can someone help me with it?

We will indeed need a test for repr. It does not need to be very complicated and in fact, something similar to what you show in the description of this pr should work. In this case, we would create a few different QobjEvo that cover most of the cases and we would check that qobjevo.__rerp__() returns the expected string. This test should go in qutip/tests/core/test_qobjevo.py. You can take a look there to see example of how the tests are written. If this is your first time writing tests you may want to take a look at our documentation and also at pytests documentation.

If the behavior of the code has changed or new feature has been added, please also update the documentation in the doc folder, and the notebook. Feel free to ask if you are not sure.

This will not be necessary this time.

Include the changelog in a file named: doc/changes/. 'type' can be one of the following: feature, bugfix, doc, removal, misc, or deprecation (see here for more information).

feature should work here.

Copy link
Member

@AGaliciaMartinez AGaliciaMartinez left a comment

Choose a reason for hiding this comment

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

thanks for your contribution! I added one suggestion but we would need some tests to be added as explained above before this gets merged. Feel free to ask any question!

qutip/core/cy/qobjevo.pyx Outdated Show resolved Hide resolved
@lklivingstone
Copy link
Contributor Author

I am working on it right now and will commit to the repo with all the changes.

@lklivingstone
Copy link
Contributor Author

@AGaliciaMartinez Hello, I have made the required changes in the repr() of QobjEvo.

Output of the new code:

>>> import qutip
>>> qutip.QobjEvo([qutip.qeye(2), lambda t: t])
<QobjEvo: dims=[[2], [2]], shape=(2, 2), type=oper, superrep=None, isconstant=False, num_elements=1>

In your suggestion, there wasnt a "<" before QobjEvo, so I added that.

I have also edited the page according to the pycodestyle.

Kindly check the test too. This is my first test, so do suggest changes, if required. I could not think of different cases, so I write 4 cases, and checked them according to the expected result.

Copy link
Member

@AGaliciaMartinez AGaliciaMartinez left a comment

Choose a reason for hiding this comment

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

I think we are close to getting it finished! good work! Lets try the following for the tests though: 1 that has a qobjevo with type=ket, superrep=None, isconstant=True, num_elements=1. And another one that has type=oper, superrep=True, isconstant=False, num_elements=2. Changig the dimensions/ shape between both examples would also be useful.

@AGaliciaMartinez
Copy link
Member

Also, regarding the <, I purposefully did not include to make the print similar to Qobj repr 😉.

@lklivingstone
Copy link
Contributor Author

@AGaliciaMartinez Thank you!
I have removed both < and > from the repr().

Now it looks like this:

>>> import qutip
>>> qutip.QobjEvo([qutip.qeye(2), lambda t: t])
QobjEvo: dims=[[2], [2]], shape=(2, 2), type=oper, superrep=None, isconstant=False, num_elements=1

I have added many tests according to your suggestions. Now there are 7 cases to check.

@lklivingstone lklivingstone marked this pull request as ready for review March 11, 2023 17:25
@coveralls
Copy link

coveralls commented Mar 12, 2023

Coverage Status

Coverage: 75.391%. Remained the same when pulling bd3da8a on lklivingstone:issue2099 into fd89a25 on qutip:master.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

Be careful when applying pycodestyle to cython code. cython has new keywords that it does not knows.

qutip/core/cy/qobjevo.pyx Outdated Show resolved Hide resolved
qutip/core/cy/qobjevo.pyx Outdated Show resolved Hide resolved
qutip/tests/core/test_qobjevo.py Outdated Show resolved Hide resolved
@lklivingstone
Copy link
Contributor Author

Changes to the test:

    case_1= repr(QobjEvo([qeye(3), lambda t: t]))
    expected_repr_1= 'QobjEvo: dims=[[3], [3]], shape=(3, 3), type=oper, superrep=None, isconstant=False, num_elements=1'
    assert case_1 == expected_repr_1

    case_2= repr(QobjEvo(qeye(2)))
    expected_repr_2= 'QobjEvo: dims=[[2], [2]], shape=(2, 2), type=oper, superrep=None, isconstant=True, num_elements=1'
    assert case_2 == expected_repr_2
    
    case_3= repr(QobjEvo(basis(5, 2)))
    expected_repr_3= 'QobjEvo: dims=[[5], [1]], shape=(5, 1), type=ket, superrep=None, isconstant=True, num_elements=1'
    assert case_3 == expected_repr_3

    X = sigmax()
    S = spre(X) * spost(X.dag())
    case_4= repr(QobjEvo(to_choi(S)))
    expected_repr_4= 'QobjEvo: dims=[[[2], [2]], [[2], [2]]], shape=(4, 4), type=super, superrep=choi, isconstant=True, num_elements=1'
    assert case_4 == expected_repr_4

    case_5= repr(QobjEvo([[qeye(4), lambda t: t], [qeye(4), lambda t: t]], compress=False))
    expected_repr_5= 'QobjEvo: dims=[[4], [4]], shape=(4, 4), type=oper, superrep=None, isconstant=False, num_elements=2'
    assert case_5 == expected_repr_5

Changes to the cython code have also been undone.

@Ericgig @AGaliciaMartinez Kindly let me know if there are any more changes to be done.

@Ericgig
Copy link
Member

Ericgig commented Mar 16, 2023

Thank you for the fixes.
Could you resolve the conflict.

@lklivingstone
Copy link
Contributor Author

@Ericgig I resolved the conversation. Do I need to do anything else?

@Ericgig
Copy link
Member

Ericgig commented Mar 16, 2023

It's not the conversation but the conflict in the qobjevo.pyx file that need to be resolved.
The PR cannot be merged while there is a conflict. Most of the tests can't be ran.

@lklivingstone
Copy link
Contributor Author

lklivingstone commented Mar 16, 2023

@Ericgig I have resolved the conflict.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

A few lines are too long, but the code is good.

@@ -241,6 +241,10 @@ cdef class QobjEvo:
if compress:
self.compress()

def __repr__(self):
cls = self.__class__.__name__
return f'{cls}: dims={self.dims}, shape={self.shape}, type={self.type}, superrep={self.superrep}, isconstant={self.isconstant}, num_elements={self.num_elements}'
Copy link
Member

Choose a reason for hiding this comment

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

This line is quite wide, can you split it?

qutip/tests/core/test_qobjevo.py Show resolved Hide resolved
@lklivingstone
Copy link
Contributor Author

I have made corrections to the lines of code which I added in the file test_qobjevo.py.
I have also changed the format of __repr__ with proper spacing.

repr:

>>> import qutip
>>> qutip.QobjEvo([qutip.qeye(2), lambda t: t])
QobjEvo: dims = [[2], [2]], shape = (2, 2), type = oper, superrep = None, isconstant = False, num_elements = 1
>>>

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

Look good, thank you.

@Ericgig Ericgig merged commit 8b12390 into qutip:master Mar 22, 2023
11 checks passed
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.

QobjEvo do not have a __repr__.
4 participants