-
Notifications
You must be signed in to change notification settings - Fork 47
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
Comments
reco.resolutions
params violate PISA "rules" for params
To be specific, the issue with |
It appears that is correct, as the computation using params is done in |
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 |
This increases complexity, though, doesn't it, both in the code and for developers to wrap their minds around? Currently, we have:
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 |
Ties in with #559 |
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
The text was updated successfully, but these errors were encountered: