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

reco.resolutions params violate PISA "rules" for params #513

Open
jllanfranchi opened this issue Nov 22, 2018 · 5 comments
Open

reco.resolutions params violate PISA "rules" for params #513

jllanfranchi opened this issue Nov 22, 2018 · 5 comments
Labels
Milestone

Comments

@jllanfranchi
Copy link
Contributor

jllanfranchi commented Nov 22, 2018

Modifying a param should modify the events accordingly, but reco.resolutions changes the events in a way that doesn't allow this behavior. Either the resolutions changes should be instantiation args (which can have such behaviors) or the handling of the params needs to be modified to follow the PISA rules for params. (this breaks PISA in odd ways in many places)

We should also verify that everyone developing stages/services for PISA is aware of these assumptions and has made the right choice between what is a class instantiation arg vs. a param

@jllanfranchi jllanfranchi changed the title reco.resolutions stage reco.resolutions params violate PISA "rules" for params Nov 22, 2018
@thehrh
Copy link
Contributor

thehrh commented Nov 23, 2018

To be specific, the issue with reco.resolutions is that its param values are only taken into account at setup time, and any later changes would silently be ignored, is that correct?

@jllanfranchi
Copy link
Contributor Author

It appears that is correct, as the computation using params is done in setup_function which (presumably?) will only be called once. It is also the case that the logic would need to be changed if it were called every time a param changed, since the computation depends on the current difference between reco and true values.

@philippeller
Copy link
Collaborator

we should probably in such a case a require such parameters to be fixed, and b) if a fixed parameter in a stage/pipeline changed re-initialize the stage/pipeline

@jllanfranchi
Copy link
Contributor Author

This increases complexity, though, doesn't it, both in the code and for developers to wrap their minds around? Currently, we have:

  • init args (i.e. every argument to the class's init besides params). These do something exactly once upon instantiation of the service. That's an "API contract" that we've long established. This also allows for anything to be passed to a service, defaults to be set, etc.
  • params As far as the service is concerned, if these are changed, then the change must be reflected in how the service runs. The long-established contract with users is that a param is something that, if it is set to a valid value, the service can do what is necessary to behave accordingly (whether that's in the beginning or after running a hundred times).

Params can be specified to be "fixed" or "free" in a config file; either case doesn't change how a service behaves, it only affects how a minimizer behaves (how it handles the params). So at the very least, the meaning of "fixed" or "free" is aliased to a different concept with your proposal.

What it sounds like you're proposing is that we have each service specify which params it requires to be fixed, and I'm not sure whether you imply that the user's choice of fixed vs. free in a config file should factor in how they are handled (or if there is an error thrown if it doesn't match up? or do you just have a new mechanism that allows for the user to do anything, but it re-initializes the entire pipeline if it is changed?).

I'd ask if you can justify having two mechanisms for what seem to be doing the exact same thing (note if you remove "init args" then it breaks backwards compatibility, so there will necessarily be two mechanisms) and adding code complexity to achieve something that's possible already?

Maybe separately: Is there a reason to have the ability to "reinitialize a pipeline" by calling some method other than __init__ on the various services in a pipeline? If that's the case, is there an in-between kind of "(re-)initialization" whereby doing what __init__ does isn't necessary? Or do you mean to truly initialize each service (i.e., by calling the class i.e. invoking __init__) all over again if a "fixed" param is changed?

@thehrh thehrh added this to the PISA 4.2 milestone Jul 30, 2024
@thehrh
Copy link
Contributor

thehrh commented Jul 30, 2024

Ties in with #559

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

No branches or pull requests

3 participants