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

Fixed Cpp issue #280

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Fixed Cpp issue #280

merged 1 commit into from
Feb 3, 2023

Conversation

osorensen
Copy link
Collaborator

No description provided.

@wleoncio
Copy link
Member

wleoncio commented Feb 3, 2023

Damn, we should have coordinated a bit. I was working on the same thing, but did the opposite (changed the other n_* variables to unsigned). 😆

IIRC, I'm afraid using int as indices for arma objects also causes warnings (not check failures, though).

@osorensen
Copy link
Collaborator Author

IIRC, I'm afraid using int as indices for arma objects also causes warnings (not check failures, though).

Are you sure? I don't think I've ever used uword or similar for arma objects, and haven't got any warnings. I also went in and looked at some of the checks for this PR, and could not find any warnings about it.

@osorensen
Copy link
Collaborator Author

osorensen commented Feb 3, 2023

Interesting to get to the bottom of this. Could not find any notes or warnings in my own build log, but trying out rhub with santizers now.

@wleoncio
Copy link
Member

wleoncio commented Feb 3, 2023

IIRC, I'm afraid using int as indices for arma objects also causes warnings (not check failures, though).

Are you sure? I don't think I've ever used uword or similar for arma objects, and haven't got any warnings. I also went in and looked at some of the checks for this PR, and could not find any warnings about it.

Couldn't confirm it on a local (Linux) compilation, so it's either not a problem anymore or just my memory tricking me. In my defense, the Arma docs state that "when using loops to access elements, it is best to use uword instead of int".

In any case, the most important thing is that all elements in the loops have the same signedness. :)

@wleoncio wleoncio linked an issue Feb 3, 2023 that may be closed by this pull request
@osorensen
Copy link
Collaborator Author

Then let's keep it this way until a warning pops up. rhub seems happy to.

I do see the point that trying to access a vector element with a negative integer will lead to failure, however, it gives a nice error message, and thus presumably does not qualify as undefined behavior(?)

Tryed it with this program:

#include <RcppArmadillo.h>
// [[Rcpp::depends(RcppArmadillo)]]

// [[Rcpp::export]]
int test(arma::mat X, int i) {
  Rcpp::Rcout << X.col(i) << std::endl;
  
  return 0;
}

And it gave this output:

> test(matrix(rnorm(6), nrow = 3), 1L)
   0.2643
   1.7759
   1.9572

[1] 0
> test(matrix(rnorm(6), nrow = 3), -1L)
Error: Mat::col(): index out of bounds

@osorensen osorensen merged commit e726347 into develop Feb 3, 2023
@osorensen osorensen deleted the uword-issue-279 branch February 3, 2023 11:48
osorensen added a commit that referenced this pull request Feb 3, 2023
* Added `plot.SMCMallows()` method + example (#114)

* Changed class of SMC outputs to SMCMallows (#114)

This makes proper dispatching of  possible, withou having any other apparent consequences.

* Adapted SMC vignette and unit tests to new `plot()` method (#114)

* Fixed CodeFactor issue

Redundant blank line at the end of a code block should be deleted.

* Gathererd SMC plot functions on same file (#114)

This should help with future DRYing, perhaps both subfunctions should eventually be internalized?

* Increment version number to 1.2.0.9004

* Updated NEWS.md

* Updated build CI config file

Using template from https://github.com/r-lib/actions/tree/v2-branch/examples#standard-ci-workflow

* Added `plot.SMCMallows()` method (#263)

* Added `plot.SMCMallows()` method + example (#114)

* Changed class of SMC outputs to SMCMallows (#114)

This makes proper dispatching of  possible, withou having any other apparent consequences.

* Adapted SMC vignette and unit tests to new `plot()` method (#114)

* Fixed CodeFactor issue

Redundant blank line at the end of a code block should be deleted.

* Gathererd SMC plot functions on same file (#114)

This should help with future DRYing, perhaps both subfunctions should eventually be internalized?

* Increment version number to 1.2.0.9004

* Updated NEWS.md

* Updated docs

* Removed duplicated function

As mentioned in the in-code comment, `scalefun()` was already defined in a different source file.

* Increment version number to 1.2.1.9001

Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com>

* Fix documentation for `plot.SMCMallows()` (#266)

* Fixed links to other package functions

Links were formatted wrongly assuming the package was setup to support markdown-formatted links. This fixes it.

* Increment version number to 1.2.1.9002

* Reverting changes to the `compute_mallows()` docs

* Reverting changes to NEWS.md

Textual change is automatically introduced by `usethis::use_dev_version()`.

* Deprecated `plot_*_posterior()` (#267)

Following the conversation started
[here](#263 (comment)),
this commit moves the superseded subfunctions of `plot.SMCMallows()`
into the `smc_mallows_deprecated.R` file for eventual removal. The
subfunctions themselves were renamed and test units to test the deprecation warnings were written.

* Matching SMC defaults to their MCMC counterparts (#269)

* Updated documentation

Some text were taken from the original implementation and make no longer sense.

* Matched SMC defaults with their original counterparts (#114)

* SMC metric defaults to footrule (#114)

* SMC leap_size defaults to 1 (#114)

* SMC alpha_prop_sd defaults to 0.5 (#114)

* SMC alpha_max defaults to 1e6 (#114)

* SMC lambda defautls to 0.1 (#114)

* Update DESCRIPTION

Incremented development version.

Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com>

* Added changes from PR #269 to NEWS.md

Those are important to mention, since they technically break backwards compatibility. I forgot to do that on he PR itself, thankfully there's a second chance. :)

* Develop (#271) (#273)

* Added `plot.SMCMallows()` method + example (#114)

* Changed class of SMC outputs to SMCMallows (#114)

This makes proper dispatching of  possible, withou having any other apparent consequences.

* Adapted SMC vignette and unit tests to new `plot()` method (#114)

* Fixed CodeFactor issue

Redundant blank line at the end of a code block should be deleted.

* Gathererd SMC plot functions on same file (#114)

This should help with future DRYing, perhaps both subfunctions should eventually be internalized?

* Increment version number to 1.2.0.9004

* Updated NEWS.md

* Updated build CI config file

Using template from https://github.com/r-lib/actions/tree/v2-branch/examples#standard-ci-workflow

* Added `plot.SMCMallows()` method (#263)

* Added `plot.SMCMallows()` method + example (#114)

* Changed class of SMC outputs to SMCMallows (#114)

This makes proper dispatching of  possible, withou having any other apparent consequences.

* Adapted SMC vignette and unit tests to new `plot()` method (#114)

* Fixed CodeFactor issue

Redundant blank line at the end of a code block should be deleted.

* Gathererd SMC plot functions on same file (#114)

This should help with future DRYing, perhaps both subfunctions should eventually be internalized?

* Increment version number to 1.2.0.9004

* Updated NEWS.md

* Updated docs

* Removed duplicated function

As mentioned in the in-code comment, `scalefun()` was already defined in a different source file.

* Increment version number to 1.2.1.9001

Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com>

* Fix documentation for `plot.SMCMallows()` (#266)

* Fixed links to other package functions

Links were formatted wrongly assuming the package was setup to support markdown-formatted links. This fixes it.

* Increment version number to 1.2.1.9002

* Reverting changes to the `compute_mallows()` docs

* Reverting changes to NEWS.md

Textual change is automatically introduced by `usethis::use_dev_version()`.

* Deprecated `plot_*_posterior()` (#267)

Following the conversation started
[here](#263 (comment)),
this commit moves the superseded subfunctions of `plot.SMCMallows()`
into the `smc_mallows_deprecated.R` file for eventual removal. The
subfunctions themselves were renamed and test units to test the deprecation warnings were written.

* Matching SMC defaults to their MCMC counterparts (#269)

* Updated documentation

Some text were taken from the original implementation and make no longer sense.

* Matched SMC defaults with their original counterparts (#114)

* SMC metric defaults to footrule (#114)

* SMC leap_size defaults to 1 (#114)

* SMC alpha_prop_sd defaults to 0.5 (#114)

* SMC alpha_max defaults to 1e6 (#114)

* SMC lambda defautls to 0.1 (#114)

* Update DESCRIPTION

Incremented development version.

Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com>

* Added changes from PR #269 to NEWS.md

Those are important to mention, since they technically break backwards compatibility. I forgot to do that on he PR itself, thankfully there's a second chance. :)

Co-authored-by: Waldir Leoncio <w.l.netto@medisin.uio.no>

Co-authored-by: Waldir Leoncio <w.l.netto@medisin.uio.no>

* Setting Cpp version to 17 (#276)

* Updated CI YAMLs, fixed lints (#278)

* Updated linter CI (#270)

Based on https://github.com/r-lib/actions/tree/v2/examples#lint-workflow

* Removed some style rules

* Fixed linting on functions and tests

* Minor rewording of SMC vignette

Resolved lint issues, replaced mentions of SMC-Mallows as an extension
to BayesMallows (since it's now part of the package).

* Updated test CI (#270)

Based on https://github.com/r-lib/actions/tree/v2/examples#test-coverage-workflow

* Explicitly loading package on SMC vignette

Also, excludes that line from the linter check

* Fixed Cpp issue (#280)

---------

Co-authored-by: Waldir Leoncio <w.l.netto@medisin.uio.no>
osorensen added a commit that referenced this pull request Mar 10, 2023
* Added `plot.SMCMallows()` method + example (#114)

* Changed class of SMC outputs to SMCMallows (#114)

This makes proper dispatching of  possible, withou having any other apparent consequences.

* Adapted SMC vignette and unit tests to new `plot()` method (#114)

* Fixed CodeFactor issue

Redundant blank line at the end of a code block should be deleted.

* Gathererd SMC plot functions on same file (#114)

This should help with future DRYing, perhaps both subfunctions should eventually be internalized?

* Increment version number to 1.2.0.9004

* Updated NEWS.md

* Updated build CI config file

Using template from https://github.com/r-lib/actions/tree/v2-branch/examples#standard-ci-workflow

* Added `plot.SMCMallows()` method (#263)

* Added `plot.SMCMallows()` method + example (#114)

* Changed class of SMC outputs to SMCMallows (#114)

This makes proper dispatching of  possible, withou having any other apparent consequences.

* Adapted SMC vignette and unit tests to new `plot()` method (#114)

* Fixed CodeFactor issue

Redundant blank line at the end of a code block should be deleted.

* Gathererd SMC plot functions on same file (#114)

This should help with future DRYing, perhaps both subfunctions should eventually be internalized?

* Increment version number to 1.2.0.9004

* Updated NEWS.md

* Updated docs

* Removed duplicated function

As mentioned in the in-code comment, `scalefun()` was already defined in a different source file.

* Increment version number to 1.2.1.9001

Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com>

* Fix documentation for `plot.SMCMallows()` (#266)

* Fixed links to other package functions

Links were formatted wrongly assuming the package was setup to support markdown-formatted links. This fixes it.

* Increment version number to 1.2.1.9002

* Reverting changes to the `compute_mallows()` docs

* Reverting changes to NEWS.md

Textual change is automatically introduced by `usethis::use_dev_version()`.

* Deprecated `plot_*_posterior()` (#267)

Following the conversation started
[here](#263 (comment)),
this commit moves the superseded subfunctions of `plot.SMCMallows()`
into the `smc_mallows_deprecated.R` file for eventual removal. The
subfunctions themselves were renamed and test units to test the deprecation warnings were written.

* Matching SMC defaults to their MCMC counterparts (#269)

* Updated documentation

Some text were taken from the original implementation and make no longer sense.

* Matched SMC defaults with their original counterparts (#114)

* SMC metric defaults to footrule (#114)

* SMC leap_size defaults to 1 (#114)

* SMC alpha_prop_sd defaults to 0.5 (#114)

* SMC alpha_max defaults to 1e6 (#114)

* SMC lambda defautls to 0.1 (#114)

* Update DESCRIPTION

Incremented development version.

Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com>

* Added changes from PR #269 to NEWS.md

Those are important to mention, since they technically break backwards compatibility. I forgot to do that on he PR itself, thankfully there's a second chance. :)

* Develop (#271) (#273)

* Added `plot.SMCMallows()` method + example (#114)

* Changed class of SMC outputs to SMCMallows (#114)

This makes proper dispatching of  possible, withou having any other apparent consequences.

* Adapted SMC vignette and unit tests to new `plot()` method (#114)

* Fixed CodeFactor issue

Redundant blank line at the end of a code block should be deleted.

* Gathererd SMC plot functions on same file (#114)

This should help with future DRYing, perhaps both subfunctions should eventually be internalized?

* Increment version number to 1.2.0.9004

* Updated NEWS.md

* Updated build CI config file

Using template from https://github.com/r-lib/actions/tree/v2-branch/examples#standard-ci-workflow

* Added `plot.SMCMallows()` method (#263)

* Added `plot.SMCMallows()` method + example (#114)

* Changed class of SMC outputs to SMCMallows (#114)

This makes proper dispatching of  possible, withou having any other apparent consequences.

* Adapted SMC vignette and unit tests to new `plot()` method (#114)

* Fixed CodeFactor issue

Redundant blank line at the end of a code block should be deleted.

* Gathererd SMC plot functions on same file (#114)

This should help with future DRYing, perhaps both subfunctions should eventually be internalized?

* Increment version number to 1.2.0.9004

* Updated NEWS.md

* Updated docs

* Removed duplicated function

As mentioned in the in-code comment, `scalefun()` was already defined in a different source file.

* Increment version number to 1.2.1.9001

Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com>

* Fix documentation for `plot.SMCMallows()` (#266)

* Fixed links to other package functions

Links were formatted wrongly assuming the package was setup to support markdown-formatted links. This fixes it.

* Increment version number to 1.2.1.9002

* Reverting changes to the `compute_mallows()` docs

* Reverting changes to NEWS.md

Textual change is automatically introduced by `usethis::use_dev_version()`.

* Deprecated `plot_*_posterior()` (#267)

Following the conversation started
[here](#263 (comment)),
this commit moves the superseded subfunctions of `plot.SMCMallows()`
into the `smc_mallows_deprecated.R` file for eventual removal. The
subfunctions themselves were renamed and test units to test the deprecation warnings were written.

* Matching SMC defaults to their MCMC counterparts (#269)

* Updated documentation

Some text were taken from the original implementation and make no longer sense.

* Matched SMC defaults with their original counterparts (#114)

* SMC metric defaults to footrule (#114)

* SMC leap_size defaults to 1 (#114)

* SMC alpha_prop_sd defaults to 0.5 (#114)

* SMC alpha_max defaults to 1e6 (#114)

* SMC lambda defautls to 0.1 (#114)

* Update DESCRIPTION

Incremented development version.

Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com>

* Added changes from PR #269 to NEWS.md

Those are important to mention, since they technically break backwards compatibility. I forgot to do that on he PR itself, thankfully there's a second chance. :)

Co-authored-by: Waldir Leoncio <w.l.netto@medisin.uio.no>

Co-authored-by: Waldir Leoncio <w.l.netto@medisin.uio.no>

* Setting Cpp version to 17 (#276)

* Updated CI YAMLs, fixed lints (#278)

* Updated linter CI (#270)

Based on https://github.com/r-lib/actions/tree/v2/examples#lint-workflow

* Removed some style rules

* Fixed linting on functions and tests

* Minor rewording of SMC vignette

Resolved lint issues, replaced mentions of SMC-Mallows as an extension
to BayesMallows (since it's now part of the package).

* Updated test CI (#270)

Based on https://github.com/r-lib/actions/tree/v2/examples#test-coverage-workflow

* Explicitly loading package on SMC vignette

Also, excludes that line from the linter check

* Fixed Cpp issue (#280)

* Updated ggplot2 code (#286)

* issue 257 (#283)

* Created validation function (#257)

* DRYing SMC aug_method checks (#257)

* Adjusted test unit expectations (#257)

* Added unit test for internal C++ function (#257)

* Increment version number to 1.2.1.9004

* Updated NEWS.md

* Removed blank line (CodeFactor nag)

* Updated package version to match master + build

* Reinstaing seed for C++ test

The `leapandshift.cpp` file contains random tests that depend on the seed.

---------

Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com>

* Improved SMC post-processing functions (#287)

* Improved validation, docs of post-processing functions (#262)

- Improved validation, docs of smc_mallows(). This is mostly to be a bit safer, since this function is not exported.
- Improved validation, docs of remaining SMC post-processing function

The validation adds a package dependency on methods, which should be fine since this is a base package.

* Added examples to post-processing functions (#262)

Used code from the vignette, then reduced `iter` and `N` for faster
computing.

* Added tests for SMC post-processing functions (#262)

* Updated NEWS.md

* Increment version number to 1.2.2.9002

* Added heat_plot function #255 (#288)

* Added heat_plot function #255

* Fixed variable bindings

* Updated NEWS.md and incremented development version

* Fixing linter issues

---------

Co-authored-by: Waldir Leoncio <w.l.netto@medisin.uio.no>
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.

C++ warnings (comparing unsigned with signed ints)
2 participants