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

implemented possibility of chosing random seed #917

Merged
merged 8 commits into from
Mar 18, 2019
Merged

implemented possibility of chosing random seed #917

merged 8 commits into from
Mar 18, 2019

Conversation

marekyggdrasil
Copy link
Contributor

No description provided.

@marekyggdrasil
Copy link
Contributor Author

#856

@Ericgig
Copy link
Member

Ericgig commented Oct 2, 2018

Thank you Marek,
It seems you forgot to add a seed variable in the definition of the rand_herm function (line 112~114). Because of this, many tests fails.
Could you add it?

@marekyggdrasil
Copy link
Contributor Author

@Ericgig sorry about that, I just added the argument, I'm also planning adding some tests but I haven't yet figured out what would be best way to do it, any suggestions welcome!

@marekyggdrasil
Copy link
Contributor Author

How about instead of checking if it is Hermitian, we could put a seed parameter and check if the output is exactly equal (or close) to given hardcoded matrix?

assert_equal(h.isherm, True)

@Ericgig
Copy link
Member

Ericgig commented Oct 5, 2018

Hardcoded matrix will cause problem in later modification of the code. (Or if numpy update it's random number generator, etc.)
Keeping the present tests, I would add something like this:

O1 = rand_herm(..., seed=seed)
O2 = rand_herm(..., seed=None)
O3 = rand_herm(..., seed=seed)
assert(O1!=O2)
assert(O1==O3)

You check that random Qobj are reproducible using a seed and work as expected when not.

@marekyggdrasil
Copy link
Contributor Author

@Ericgig good idea about the tests, I'll implement them in this way

now there seems to be some problem with continuous integration, it fails on

ERROR: Failure: ImportError (libgfortran.so.1: cannot open shared object file: No such file or directory)

I attempted to fix it by installing libgfortran using conda, however it didn't fix the issue

travis-ci/travis-ci#4816 (comment)
menpo/landmarkerio-server#23 (comment)

I don't want to mess up too much with continous integration as it takes long time to rebuild it, any suggestions on that?

@Ericgig
Copy link
Member

Ericgig commented Oct 9, 2018

Do worry about the failing python2.7 tests, it's a common error on all recent checks. Your pull should not affect them.

@marekyggdrasil
Copy link
Contributor Author

@Ericgig tests implemented and passing, please code review if you can.

@Ericgig
Copy link
Member

Ericgig commented Oct 16, 2018

As it is in your code, if no seed is given, then np.random.seed(None) is called. np.random.seed(None) reset the seed based on the computer time.
It would break the reproducibility of code which already set the seed once:

np.random.seed(seed)
my_rand_qobj = qt.rand_herm(N)

It would be better if you did:

if seed is not None:
    np.random.seed(seed=seed)

@marekyggdrasil
Copy link
Contributor Author

@Ericgig thanks for suggestion

weird, my local tests all passed, let me examine the problem

@Ericgig
Copy link
Member

Ericgig commented Oct 19, 2018

It seems to be caused by a new version of cython which affect which files are used for tests by nose. I made a patch.

@marekyggdrasil
Copy link
Contributor Author

Thats great, thanks @Ericgig

If any other modifications requested don't hesitate to let me know. Looking forward to have my changes merged!

@Ericgig Ericgig self-assigned this Feb 21, 2019
@coveralls
Copy link

coveralls commented Feb 28, 2019

Coverage Status

Coverage increased (+0.05%) to 72.428% when pulling 444bcfd on marekyggdrasil:master into dbbd113 on qutip:master.

qutip/random_objects.py Outdated Show resolved Hide resolved
qutip/random_objects.py Outdated Show resolved Hide resolved
qutip/random_objects.py Outdated Show resolved Hide resolved
@Ericgig
Copy link
Member

Ericgig commented Feb 28, 2019

I marked a few spot where the seed was applied more than once.
The failed test is a known issue.

@Ericgig Ericgig added the review in progress PR being reviewed label Feb 28, 2019
@marekyggdrasil
Copy link
Contributor Author

@Ericgig I marked your remarks as resolved and made some new changes. If anything more required don't hesitate to ping me.

@Ericgig Ericgig merged commit 73a2797 into qutip:master Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review in progress PR being reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants