Fix typos and clarify sample site naming guidance in developer_documentation.rst
#368
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #368 +/- ##
=======================================
Coverage 93.24% 93.24%
=======================================
Files 39 39
Lines 918 918
=======================================
Hits 856 856
Misses 62 62
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| - Composing ``Model`` objects is discouraged. To modularize and reuse code across ``Model``s, create custom ``RandomVariable`` classes. | ||
| - Returning anything from ``Model.sample`` is discouraged. Instead, sample from models using ``Predictive`` or our ``prior_predictive`` or ``posterior_predictive`` functions. | ||
| - Using ``numpyro.deterministic`` within a ``RandomVariable.sample`` method is discouraged. Only use it in ``Model.model`` calls. If something might need to be recorded from a ``RandomVariable``, it should be returned from the ``RandomVariable.sample`` call so it can be recorded within the ``Model.model`` call. | ||
| - Using default site names in a ``RandomVariable.sample`` is discouraged. Only use default site names at the ``Model.model`` level. |
There was a problem hiding this comment.
I think there is a better term than “using.” This is meant to suggest that someone writing a random variable should not give their own random variable a default site name. We should say something closer to that and point out that the site name should be an argument to the RV constructor.
There was a problem hiding this comment.
Tried something in 33063a7. Might be too verbose. Let me know what you think, @damonbayer
dylanhmorris
changed the title
developer_documentation.rstdeveloper_documentation.rst
| --------------------------- | ||
| - Use the ``data_`` prefix for (potentially) observed data. | ||
| - Use the ``_rv`` suffix for random variables. | ||
| - Use the ``observed_`` for the output of sample statements where ``obs`` is a ``data_`` prefixed object. |
There was a problem hiding this comment.
I am not entirely sure that these prefixes make complete sense. Particularly, I make an explicit distinction between observed and unobserved data. In principle, everything can be called data, so it is superfluous to use that as a prefix. Instead, I would say sampled_ for any output (deterministic or random), leave the observed_ to actually observed data, and keep the _rv suffix.
There was a problem hiding this comment.
@gvegayon @dylanhmorris How about this? The observed_ prefix is nice because it maps to the obs argument in numpyro.sample, though I understand why it is confusing to say the observed_ object may not actually be observed.
- Use the ``observed_`` prefix for (potentially) observed data.
- Use the ``_rv`` suffix for random variables.
- Use the ``sampled_`` for the output of sample statements where ``obs`` is a ``obserced_`` prefixed object.
- Thus, code which may reasonably written like ``infections = infections.sample(x, obs=infections)`` should instead be written ``sampled_infections = infections_rv.sample(x, obs=observed_infections)``.
|
Closing pending a rethink of sample site naming etc in #407 |
No description provided.