-
Notifications
You must be signed in to change notification settings - Fork 40
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
Retire deprecated Options classes #649
Conversation
Remove classes SequenceStatisticalOptions and MetropolisHastingsSGOptions. Required writing new copy constructor so MhOptionsValues could be built from an MLSampleLevelOptions object.
Cleanup more deprecated code related to SequenceStatisticalOptions and MetropolisHastingsSGOptions There remains some more unreachable code, but a great many errors crop up when enabling QUESO_USES_SEQUENCE_STATISTICAL_OPTIONS, so not going to ferret them all out right now.
Actually one outstanding question: can code protected by
be removed? SsOptionsValues in particular? That preprocessor define is hardwired off in Defines.h, but associated code is not marked deprecated, so I left it in place, although didn't test as flipping that define caused all manner of chaos. |
Codecov Report
@@ Coverage Diff @@
## dev #649 +/- ##
=========================================
+ Coverage 74.85% 75.15% +0.3%
=========================================
Files 312 312
Lines 23794 23692 -102
=========================================
- Hits 17810 17805 -5
+ Misses 5984 5887 -97
Continue to review full report at Codecov.
|
That define looks like it's been hardwired since 2012, and we're obviously not supporting it, so I'd say strip it out, but let's wait to hear from @dmcdougall to confirm first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd have factored into commits slightly differently, but the end result looks good and the intermediate commit passes "make check" too so I'm happy.
Yes, I confirmed it passed in between. I would have done differently as well, but had missed some code on my first pass (due to conditional compilation) and it didn't seem worth a careful revert and re-apply. I'm thinking I'll merge this PR and then pick up the sequence stat options based on Damon's feedback. |
Remove classes SequenceStatisticalOptions and
MetropolisHastingsSGOptions. Required writing new copy constructor so
MhOptionsValues could be built from an MLSampleLevelOptions object.
This to be merged to dev prior to completion of #569 I don't think there's anything too controversial to request review on, but wouldn't mind a quick look. Some of the code will cleanup as I finish up #569