Skip to content

Add version header for CmdStan#1231

Merged
mitzimorris merged 1 commit intodevelopfrom
cmdstan-version-header
Jan 4, 2024
Merged

Add version header for CmdStan#1231
mitzimorris merged 1 commit intodevelopfrom
cmdstan-version-header

Conversation

@andrjohns
Copy link
Contributor

Submisison Checklist

  • Run tests: ./runCmdStanTests.py src/test
  • Declare copyright holder and open-source license: see below

Summary:

This PR adds a version.hpp header following the conventions of the Stan and Math libraries

Intended Effect:

The CmdStan version is currently only recorded in the Makefile, which means that any downstream users need to also include the makefiles in order to identify the version of the CmdStan headers & sources.

This allows downstream users to identify the version of the CmdStan headers/sources without needing to include & parse the makefile, improves consistency with the Stan & Math libraries

How to Verify:

Side Effects:

Documentation:

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Andrew Johnson

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

Copy link
Member

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

Seems useful. Thoughts on having this be the version we put in the CSV files? Code is here:
https://github.com/stan-dev/cmdstan/blob/develop/src/cmdstan/write_stan.hpp

@WardBrian
Copy link
Member

Also tagging @serban-nicusor-toptal so he would know that there is a new file that needs updating each release!

@rok-cesnovar
Copy link
Member

Seems useful. Thoughts on having this be the version we put in the CSV files?

I don't see a case where the cmdstan and stan versions would diverge, but it depends on what we want to write in the file, the Stan version of the interface version. I think the Stan version?

@WardBrian
Copy link
Member

I don't see a case where the cmdstan and stan versions would diverge

They've diverged right now (CmdStan is on 2.33.1, Stan on 2.33.0) because the latest bugfix release didn't require any changes to Stan or Math

@rok-cesnovar
Copy link
Member

Oh right. I don't really have a preference honestly, was just being nitpicky :)

@WardBrian
Copy link
Member

I think given a cmdstan version, there is always a unique version of Stan shipped with that (we never update Stan without updating CmdStan) but the inverse need not be true (e.g., right now), so it seems reasonable to include the cmdstan version in the csv. It's not a big deal either way, I think

@mitzimorris mitzimorris merged commit b62133a into develop Jan 4, 2024
@WardBrian WardBrian deleted the cmdstan-version-header branch January 4, 2024 15:12
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 this pull request may close these issues.

4 participants

Comments