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

Unify constructors to always allow a .stan file to be passed #188

Merged
merged 4 commits into from
Nov 30, 2023

Conversation

WardBrian
Copy link
Collaborator

@WardBrian WardBrian commented Nov 29, 2023

Closes #187.

Changes:

  • Python: StanModel.from_stan_file is deprecated. The logic is subsumed in the constructor, which will trigger compilation if the file provided ends in .stan
  • Python: The model_data argument is renamed to data. The previous argument still works but issues a warning.
  • Python: All functions which accepted strings for file paths now also accept the os.PathLike interface
  • Julia: The StanModel(;stan_file,...) overload is deprecated. This logic is now subsumed in the constructor, which will trigger compilation if the file provided ends in .stan

Caused by loading non-THREADS model in same instance as THREADS=1
Copy link
Owner

@roualdes roualdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great. Just a general questions about putting deprecation warnings in doc strings, to compliment error messages.

model_lib: str,
model_data: Optional[str] = None,
model_lib: Union[str, PathLike],
data: Union[str, PathLike, None] = None,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is None better here than ""?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None ends up being passed as nullptr to C++, which then creates an empty_var_context:

https://github.com/roualdes/bridgestan/blob/41b66b7956817ecb411e7dccedeed4d6fd8347dc/src/model_rng.cpp#L65C1-L67C56

If you pass "" instead, this string is then treated as an empty JSON, which creates a json parser/var_context and populates it with nothing.

So they're semantically equivalent, but trigger different code paths

python/bridgestan/model.py Outdated Show resolved Hide resolved
python/bridgestan/model.py Show resolved Hide resolved
@WardBrian WardBrian requested a review from roualdes November 30, 2023 19:13
Copy link
Owner

@roualdes roualdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@WardBrian WardBrian merged commit 7ca2952 into main Nov 30, 2023
19 checks passed
@WardBrian WardBrian deleted the cleanup/unify-construction-logic branch November 30, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More consistent instantiation from a .stan file in various languages
2 participants