stan-dev / stan

Stan development repository. The master branch contains the current release. The develop branch contains the latest stable development. See the Developer Process Wiki for details.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
2.57k stars 368 forks source link

Remove old/unused IO code #3177

Closed WardBrian closed 1 year ago

WardBrian commented 1 year ago

Submission Checklist

Summary

Removes unused code from stan/io:

Updated a few tests to reflect

Intended Effect

Removes old code no longer needed

How to Verify

Side Effects

Documentation

None

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):

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

WardBrian commented 1 year ago

It seems like a bunch of tests were relying on the fact that model_header.hpp was including io/dump (which models never depend on!) and did not include it themselves. I'm updating those tests now

bgoodri commented 10 months ago

I was using the reader in the the last section of the StanHeaders vignette. I was eventually able to figure out that I needed to use deserializer, but just because something is not used within Stan doesn't mean it is not used by someone writing their own C++ on top of Stan. We are trying to encourage more of that, but breaking their code does not help.

WardBrian commented 10 months ago

We are trying to encourage more of that

I thought we were trying to encourage less of that, at least in the R world?

My understanding is that Stan had not had any internal usages of this code in the 4 versions prior to when we removed it. We really have no way of knowing what other usages exist for something like this

bgoodri commented 10 months ago

I agree it is impossible to know how stuff in Stan Math or Library may be used in external C++ projects (unless they complain when stuff breaks). But other C++ header libraries like Boost and Eigen won't just remove stuff in a point release for that reason.

WardBrian commented 10 months ago

Boost has had at least one breaking change in each of 1.80, 1.81, and 1.82. Most (but not all) of these seem to have had at least 1 version's worth of warnings, which we could also do at the C++ level here, assuming that consumers of the C++ never jump more than a couple versions at a time

We probably should clarify the exact versioning scheme of each repo and how it relates to the others. So far, everything except math has been kept in lock-step, but I believe the primary backwards compatibility concerns have been at the source level of the language, rather than cmdstan/stan/etc