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

Backwards compatibility: We should allow functions to be named _jacobian that don't use the new feature to be called anywhere for a few versions #1470

Closed
WardBrian opened this issue Nov 25, 2024 · 4 comments · Fixed by #1471

Comments

@WardBrian
Copy link
Member

We took care to implement jacobian+= in a backwards-compatible way, but not _jacobian functions, of which there are some in the wild: https://github.com/search?q=language%3Astan+_jacobian%28&type=code

Because these functions don't need the jacobian += feature, ideally they would continue to be treated as FnPlains, with a warning. I'm not sure if this is actually implementable, however, due to the order-independence of the functions block.

First reported by @tillahoffmann https://discourse.mc-stan.org/t/planning-the-2-36-release/36382/15?u=wardbrian

@WardBrian
Copy link
Member Author

@nhuurre -- do you have any sense of how this could be done in a way that isn't so horrible?

tillahoffmann added a commit to onnela-lab/gptools that referenced this issue Nov 25, 2024
tillahoffmann added a commit to onnela-lab/gptools that referenced this issue Nov 25, 2024
tillahoffmann added a commit to onnela-lab/gptools that referenced this issue Nov 25, 2024
@WardBrian
Copy link
Member Author

Options:

  • we find a way to typecheck that allows both usages
  • we don't allow _jacobian functions for 3 versions while we emit warnings, and then we do what we do now
  • we just re-use the _lp function name?

Also pinging @SteveBronder

@nhuurre
Copy link
Collaborator

nhuurre commented Nov 26, 2024

I think the easiest backwards-compatibility hack would be that right before calling Typechecker.add_userdefined_function you check if any of the user-defined _jacobian functions actually use jacobian += and if not, then frontent never creates FnJacobian, only FnPlain, ie, replace every Fun_kind.suffix_from_name in Typechecker with

let jacobian_compatibilty_mode = ref false
let suffix_from_name name =
  match Fun_kind.suffix_from_name name with
  | FnJacobian when !jacobian_compatibilty_mode -> Fun_kind.FnPlain
  | f -> f

Trying to do this on a per-function basis gets complicated if there are mutually recursive _jacobian functions.

@WardBrian
Copy link
Member Author

I agree. I had just finished sketching out a graph search over the functions (while not entirely knowing how to resolve overloads to do so), when it occurred to me that for backwards compatibility purposes a global check is sufficient. If we see the new feature, we don’t have to worry about this being an old model…

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

Successfully merging a pull request may close this issue.

2 participants