TuringLang / AbstractPPL.jl

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

added dispatched 'subsumes(parent::Symbol, child::Symbol)'. #83

Closed YongchaoHuang closed 1 year ago

YongchaoHuang commented 1 year ago

added the following dispatched method for 'subsumes':

function subsumes(parent::Symbol, child::Symbol)
    parent_str = string(parent)
    child_str = string(child)
    if parent_str == child_str
        return true
    end
    if length(parent_str) > length(child_str)
        return false
    end
    return child_str[1:length(parent_str)] == parent_str
end

This method is supposed to be used in the PR for DynamicPPL.jl: #438 log probability interface for post-inference analysis.

sunxd3 commented 1 year ago

There is a function that serves the same functionality already existed: https://github.com/TuringLang/DynamicPPL.jl/blob/b23acff013a9111c8ce2c89dbf5339e76234d120/src/varname.jl#L1-L16 Try using this instead?

For now, let's keep the function where it is. If the demand for the function is repeated in the future, we can consider moving it into APPL.

torfjelde commented 1 year ago

We had this sort of functionality in here before but we removed it because we really shouldn't be doing subsumes on strings as it's just too unreliable.

The function in DPPL mentioned above also, IMO, shouldn't be used; it's only there as a "hack".

Why do we need this function @YongchaoHuang ? (Probably better to explain this in the linked PR rather than here)

YongchaoHuang commented 1 year ago

There is a function that serves the same functionality already existed: https://github.com/TuringLang/DynamicPPL.jl/blob/b23acff013a9111c8ce2c89dbf5339e76234d120/src/varname.jl#L1-L16 Try using this instead?

For now, let's keep the function where it is. If the demand for the function is repeated in the future, we can consider moving it into APPL.

That's good to know. Yes both solutions are doing the same thing. 'subsumes_string' is rather restrictive, as warned in its docstring. It requires supplying 'u_indexing' which is set by default 'u'['', this would be fine for naming conventions such as 'x[1]' (as is the case in 'varname.jl'), however, for variable names such as 'x_1', you will need to explicitly supply 'u_indexing=x'_'', etc. I think 'subsumes_sym' compares the strings in a slightly wise way - it first checks the lengths, then further compares the first several strings without concerning about the indexing thing.

However, as @torfjelde and @devmotion mentioned, we need to avoid string to string comparison 'hack' method, as this is fragile. If this is the case, I will just delete this PR, and find alternative solution for PR#438.

YongchaoHuang commented 1 year ago

@torfjelde Again, it's good to know the historic opinion on 'subsumes'. So we really shouldn't be doing string to string comparison, but need to compare the inner structure of the parent and child objects. I shall delete this PR and try solving PR#438 in a more principled way. Many thanks for pointing this out.

sunxd3 commented 1 year ago

Follow the discussion, we decided not to add string based subsume function due to its instability.