Skip to content
This repository has been archived by the owner on Aug 13, 2018. It is now read-only.

Meaningless recursion in pytram Estimator base class #46

Open
franknoe opened this issue Jul 23, 2015 · 2 comments
Open

Meaningless recursion in pytram Estimator base class #46

franknoe opened this issue Jul 23, 2015 · 2 comments

Comments

@franknoe
Copy link
Contributor

in the Estimator base class we have:

    @property
    def pi_i(self):
        return np.exp(-self.f_i)

    @property
    def f_i( self ):
        return -np.log(self.pi_i)

and

    @property
    def pi_K_i(self):
        return np.exp(-self.f_K_i)

    @property
    def f_K_i(self):
        return -np.log(self.pi_K_i)

which are endless recursions and don't do anything. I guess they are effectively never called because they are overridden in the base class, but then at least one of the two should have no implementation and raise a NotImplementedError when called.

@cwehmeyer
Copy link
Member

Depending on the estimator derived from this (abstract) class, it is one or the other version which is overridden. I think this approach is reasonable since we do not need to implement things twice.

@franknoe
Copy link
Contributor Author

I understand the logic, but I probably wouldn't do it that way because it's dangerous when new Estimators are developed without understanding this logic.
I think a good default behavior is to expect that the estimator computes the energies and always use the energies as a default internal representation as this is numerically stable. The stationary distributions can then be computed robustly from the energies. So one would implement the stationary distributions in the base class and only give an empty implementation of the energy-properties there that has to be overriden.

Anyway this is low-prio.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants