TuringLang / AbstractPPL.jl

Common types and interfaces for probabilistic programming
http://turinglang.org/AbstractPPL.jl/
MIT License
27 stars 7 forks source link

Prettier show for `Colon` #22

Closed torfjelde closed 3 years ago

torfjelde commented 3 years ago

Currently:

julia> @varname(x[:,1])
x[Colon(),1]

On this branch:

julia> @varname(x[:,1])
x[:,1]

Seems like the latter is preferable, and will make a lot of downstream printing much nicer, e.g. chains obtained in Turing.

Only potential issue is that people might have code that access elements in a chain, etc. using Colon() in the string-rep, so maybe this should be a breaking release?

torfjelde commented 3 years ago

I'll make it a breaking release.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1018107869


Totals Coverage Status
Change from base Build 854623890: 0.6%
Covered Lines: 49
Relevant Lines: 59

💛 - Coveralls
codecov[bot] commented 3 years ago

Codecov Report

Merging #22 (e780a50) into main (34fc3e1) will increase coverage by 0.59%. The diff coverage is 100.00%.

:exclamation: Current head e780a50 differs from pull request most recent head a01fa61. Consider uploading reports for the commit a01fa61 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
+ Coverage   82.45%   83.05%   +0.59%     
==========================================
  Files           1        1              
  Lines          57       59       +2     
==========================================
+ Hits           47       49       +2     
  Misses         10       10              
Impacted Files Coverage Δ
src/varname.jl 83.05% <100.00%> (+0.59%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 34fc3e1...a01fa61. Read the comment docs.

torfjelde commented 3 years ago

Eehh maybe we should require tests to pass before merging here too? The tests are now failing (I forgot to rename some stuff in the tests).

phipsgabler commented 3 years ago

@torfjelde sounds like a good idea. Where can I activate that?