QInfer / python-qinfer

Library for Bayesian inference via sequential Monte Carlo for quantum parameter estimation.
BSD 3-Clause "New" or "Revised" License
92 stars 31 forks source link

WIP: Citation metadata w/ duecredit #129

Closed cgranade closed 7 years ago

cgranade commented 7 years ago

This WIP PR addresses #128 by adding citation metadata to __init__.py. This citation is guarded with the tag ["implementation"], with the idea that citations to underlying theory could be added to individual functions with the ["theory"] tag.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.02%) to 71.764% when pulling ab90ed783028460a8404e9ce8c17e55010f584b2 on feature-duecredit into 75f8723d0cb26199e90fdb426f654a80ab70e7a0 on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.07%) to 71.851% when pulling e480a9ece819e45541d327b0c53789e941d032ee on feature-duecredit into 75f8723d0cb26199e90fdb426f654a80ab70e7a0 on master.

ihincks commented 7 years ago

Looking good, Chris. Are you thinking of implementations=experimental, or having both? It would also be good to add something to the guide about how to use it.

cgranade commented 7 years ago

Thanks! My thought was to have tags broken down into theory and implementations, with the latter being included by duecredit by default, but I really like your suggestion of experiment as well to point to experimental applications. I'll start adding to the guide on how to use the new duecredit functionality soon, with the explicit note that the bibliography is not yet complete and that users should still be careful to cite as appropriate.

cgranade commented 7 years ago

One weird bug so far: duecredit injects "Frey, B.J. & Dueck, D., 2007. Clustering by Passing Messages Between Data Points. Science, 315(5814), pp.972–976." as a citation for a part of scikit-learn's clustering library, such that that citation shows up if the user has scikit-learn installed. I'm not sure quite what to do about that, since QInfer definitely does import sklearn.cluster in clustering.py.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 71.905% when pulling b874b1ceef92b8a9a4a9b0af28e2b22f203ea721 on feature-duecredit into 75f8723d0cb26199e90fdb426f654a80ab70e7a0 on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 71.905% when pulling 89da0fede3fca3f7ca0d72ed0f6fdbf2c2a44164 on feature-duecredit into 75f8723d0cb26199e90fdb426f654a80ab70e7a0 on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 71.905% when pulling 7d8d92a80111671e832741bb6d0f5ec40bde5e23 on feature-duecredit into 75f8723d0cb26199e90fdb426f654a80ab70e7a0 on master.

ihincks commented 7 years ago

Let me know when you satisfied with the state of this PR. I will test it on my machine, and subsequently provide a code review.

cgranade commented 7 years ago

I think I'm mostly happy with this PR now, modulo that the process listed in the docs for generating a bibliography using the DUECREDIT_ENABLE="yes" environment variable setting seems to not always work. I'm a bit confused as to exactly what's going on there, and it could well be upstream, but if you have any insight I'd greatly appreciate it. Thanks! ♥

ihincks commented 7 years ago

Played around briefly, seems to be working. If I just import qinfer as qi without anything else (whether using env variable or with python -m duecredit), I end up with

[4] 2017-04-03 11:16:13,396 [WARNING] DueCredit internal failure while running <function _dump_collector_summary at 0x7fc3c2755aa0>: OSError(13, 'Permission denied'). Please report to developers at https://github.com/duecredit/duecredit/issues (utils.py:199)

Do you get this too?

I am also wondering about jupyter notebooks. It seems that you can get the current state of collection using:

import os
os.environ['DUECREDIT_ENABLE'] = 'yes'
import qinfer as qi
qi.due._dump_collector_summary()

Although from my usual use of qinfer (through notebooks, possibly with local modules) this seems convenient, I would hesitate to recommend people use hidden functions.

ihincks commented 7 years ago

Submitted issue: https://github.com/duecredit/duecredit/issues/104 and: https://github.com/duecredit/duecredit/issues/105

cgranade commented 7 years ago

Thanks for looking at that! I didn't get that error, but rather intermittently got a UnicodeDecodeError. I'll isolate a test case and file it as an issue with DueCredit as well.

ihincks commented 7 years ago

Hmm, the test script

import qinfer as qi
m = qi.CoinModel()
prior = qi.UniformDistribution([[0,1]])
u = qi.SMCUpdater(m, 1000, prior)
u.resampler(m, u.particle_weights, u.particle_locations)

is not putting Lui and West in the list. Am I doing something wrong? Just running python -m duecredit test.py.

cgranade commented 7 years ago

The @dcite annotation for the Liu–West resampler uses the ["theory"] tag, which is disabled by default. Does the test script work with DUECREDIT_REPORT_TAGS=*? Also, pursuant to the discussion above, it might be a good idea to just change theory to implementation anyway.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 71.905% when pulling fa763f15c70ad0d6f51f97619bfedf967adc3f9e on feature-duecredit into 75f8723d0cb26199e90fdb426f654a80ab70e7a0 on master.