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

Scope separator for netcdf #5663

Merged
merged 7 commits into from
Mar 30, 2022
Merged

Scope separator for netcdf #5663

merged 7 commits into from
Mar 30, 2022

Conversation

ferrine
Copy link
Member

@ferrine ferrine commented Mar 28, 2022

resolves #5657

Breaking changes

Use / separator for model scopes

Before #5607After
with pm.Model("sub") as model:
    b = pm.Normal("var")
model.named_vars.keys()
# dict_keys(['sub_var'])
with pm.Model("sub") as model:
    b = pm.Normal("var")
model.named_vars.keys()
# dict_keys(['sub|var'])

Fix an error for nesting

Before #5607After
with pm.Model("sub") as model:
    b = pm.Normal("var")
    with pm.Model("sub") as model:
        b = pm.Normal("var")
# Error
with pm.Model("sub") as model:
    b = pm.Normal("var")
    with pm.Model("sub") as sub_model:
        b = pm.Normal("var")
model.named_vars.keys()
# dict_keys(['sub|var', 'sub|sub|var'])

It is also possible to save to netcdf

with pm.Model("scope") as model:
    b = pm.Normal("var")
    trace = pm.sample()
az.to_netcdf(trace, "trace.nc")
trace1 = az.from_netcdf("trace.nc")
assert "scope|var" in trace1.posterior

@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #5663 (82df819) into main (fa015e3) will decrease coverage by 3.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5663      +/-   ##
==========================================
- Coverage   86.86%   83.72%   -3.15%     
==========================================
  Files          75       75              
  Lines       13715    13722       +7     
==========================================
- Hits        11914    11489     -425     
- Misses       1801     2233     +432     
Impacted Files Coverage Δ
pymc/model.py 85.61% <100.00%> (+<0.01%) ⬆️
pymc/distributions/transforms.py 54.12% <0.00%> (-45.88%) ⬇️
pymc/distributions/dist_math.py 57.71% <0.00%> (-29.72%) ⬇️
pymc/distributions/logprob.py 68.80% <0.00%> (-26.61%) ⬇️
pymc/data.py 58.82% <0.00%> (-24.79%) ⬇️
pymc/tuning/scaling.py 35.18% <0.00%> (-24.08%) ⬇️
pymc/distributions/continuous.py 87.87% <0.00%> (-10.31%) ⬇️
pymc/aesaraf.py 79.39% <0.00%> (-9.05%) ⬇️
pymc/distributions/discrete.py 91.48% <0.00%> (-8.25%) ⬇️
pymc/distributions/multivariate.py 86.61% <0.00%> (-5.43%) ⬇️
... and 6 more

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

It's not as nice :(

@ferrine
Copy link
Member Author

ferrine commented Mar 28, 2022

Well, | seems to be the least ugly otherwise. I'll wait for others to approve or propose a better separator. Possible candidates are:

::

scope::variable
scope::sub::variable

:

scope:variable
scope:sub:variable

#

scope#variable
scope#sub#variable

@ricardoV94
Copy link
Member

Those actually look a bit better. What about > ?

My problem with the | is that it makes me thing of or too much

@ferrine
Copy link
Member Author

ferrine commented Mar 28, 2022

Those actually look a bit better. What about > ?

My problem with the | is that it makes me thing of or too much

scope>variable
scope>sub>variable

It compares well to the rest, what do you think of ::, so far one of the best candidates for me?

@ricardoV94
Copy link
Member

Which one do you prefer?

@ferrine
Copy link
Member Author

ferrine commented Mar 28, 2022

::

scope::variable
scope::sub::variable

@ferrine
Copy link
Member Author

ferrine commented Mar 28, 2022

it is at least pytest like

@michaelosthege
Copy link
Member

All separators | / \ . > → :: will require to access with idata.posterior["sub|var"] instead of idata.posterior.sub_var.
That's manageable I guess.

But are there consequences for doing ArviZ things with such variable names? Will it break?

Note that if we'd (finally) introduce a pm.rcParams (#4657), the choice of separator would be a prime candidate.

@ferrine
Copy link
Member Author

ferrine commented Mar 29, 2022

All separators | / \ . > → :: will require to access with idata.posterior["sub|var"] instead of idata.posterior.sub_var.

Yes, but with _ separator you can't tell if it is two words or a scope and a variable name

@ferrine
Copy link
Member Author

ferrine commented Mar 30, 2022

But are there consequences for doing ArviZ things with such variable names? Will it break?

I'm using it with arviz just fine, there were no troubles so far

@ferrine ferrine merged commit c22ea96 into main Mar 30, 2022
@ferrine ferrine deleted the scope-separator branch March 30, 2022 09:44
@ferrine
Copy link
Member Author

ferrine commented Mar 30, 2022

I've merged this for now to enable saving to disk. Agree that with #4657 it makes sense to specify the separator in rcParams.

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.

az.to_netcdf errors in variables containing / in names
3 participants