The _update, _reset, _required_parameters methods (and _get_derived_parameters to a lesser degree) add a lot of boiler plate but in most cases don't actually do anything any more, as shown by the many "This implementation has nothing to do."

[marcpaterno]

    The `_update`, `_reset`, `_required_parameters` methods (and `_get_derived_parameters` to a lesser degree) add a lot of boiler plate but in most cases don't actually do anything any more, as shown by the many `"This implementation has nothing to do."`

I'd like to suggest removing the @abstractmethod for these in Updatable, or at least move the default implementations to places like Systematic and Source. I imagine the most common contributions to firecrown will be in the form of new systematics by users that neither understand nor care about the very neat internals you've come up with. I believe that allowing users to write a simple Systematics class with just an __init__ and apply method makes for a more welcoming environment and readable code than having to copy in a bunch of empty _* methods that most users won't ever need to change.

The existence of these methods should be documented for advanced users but even knowing what these methods do, I personally struggle to come up with a use case right now for anything but _get_derived_parameters.

Originally posted by @tilmantroester in #194 (comment)

[vitenti]

@tilmantroester Our original idea was to make those methods abstract to make the user to think about their object structure to be sure they are not missing anything when implementing a new object. In practice, these methods are basically handling the calls to update the parameters/get a list of names/reset (that's now done automatically) and to call update/required_parameters/reset on any other Updatable object contained in the object's instance.

We have the option to make all automatic, we can make a similar procedure where we inspect all properties in a object and save those which are Updatable/UpdatableCollection and automatically trigger the matching methods. However, we are worried if this would make the objects too "magical", that is, difficult to understand to the final user. Do you have an opinion on this? Should we ask on slack?

[tilmantroester]

That's an interesting question. I think it's nice to have good default behaviour (such as automatically calling the update/reset/required_parameters/get_derived_parameters methods of sub-Updatables), as long as it can be customized (for example, if an objects contains an Updatable that shouldn't be updated for some reason). But my feeling is that at this point, such an all automatic tracking would nice to have but not critical.

Where the existence of nested Updatable is implied by the API (for example the systematics list in Source), having a default implementation in Source would make sense, rather then copying the same code in NumberCounts, WeakLensing, and potential future sources, like tSZ. But in general I think that if an object is complex enough to include other Updatables, then it's reasonable for the user to implement _update, _reset, and _required_parameters.
Many objects, such as systematics and simple statistics (like supernova), probably won't include other Updatables at all that need to kept track of though, where the default behaviour of not doing anything implemented in a parent class would be sufficient.

[marcpaterno]

This issue is partly solved with PR #275. That does not complete the work, and we’ll leave this issue open until that work is completed.

[vitenti]

@marcpaterno can you confirm that this was fixed by #310 ? If so, please close this issue.

[marcpaterno]

We believe that #310 has fixed this issue. If there is additional redesign needed please open a new targeted issue.