Skip to content

Commit

Permalink
solve CRAN complaints, refactor error printing
Browse files Browse the repository at this point in the history
  • Loading branch information
david-cortes committed Nov 28, 2023
1 parent b29a11a commit 3ee06a0
Show file tree
Hide file tree
Showing 12 changed files with 66 additions and 154 deletions.
2 changes: 1 addition & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Package: cmfrec
Type: Package
Title: Collective Matrix Factorization for Recommender Systems
Version: 3.5.1-1
Version: 3.5.1-2
Authors@R: c(
person(given="David", family="Cortes", role=c("aut", "cre", "cph"),
email="[email protected]"),
Expand Down
2 changes: 1 addition & 1 deletion LICENSE
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
MIT License

Copyright (c) 2020-2022 David Cortes
Copyright (c) 2020-2023 David Cortes

All rights reserved.

Expand Down
42 changes: 6 additions & 36 deletions R/fit.R
Original file line number Diff line number Diff line change
Expand Up @@ -113,42 +113,12 @@ NULL
#' of internal BLAS threads before entering a multi-threaded region, in order to avoid oversubscription of
#' threads. This can become an issue when using OpenBLAS if it is the 'pthreads' variant.
#'
#' This package relies heavily on BLAS and LAPACK functions. For better performance, it is
#' recommended to use an optimized backed for them, such as MKL or OpenBLAS.
#'
#' In Windows, the easiest way of getting MKL is to use Microsoft's
#' \href{https://mran.microsoft.com}{MRAN distribution of R}, while OpenBLAS can be obtained by
#' following \href{https://github.com/david-cortes/R-openblas-in-windows}{this tutorial}
#' (no new R installation required).
#'
#' In Linux, these can be installed through the system's package manager. In Debian and
#' Debian-based distributions such as Ubuntu, the default BLAS and LAPACK can be configured
#' through the alternatives system (see the
#' \href{https://wiki.debian.org/DebianScience/LinearAlgebraLibraries}{Debian docs} or
#' \href{https://stackoverflow.com/a/49842944/5941695}{this post for MKL}).
#'
#' By default, in a regular x86-64 CPU, R will compile all packages with generic options
#' `-msse2` and `-O2`, which misses lots of performance optimizations, and in particular,
#' `cmfrec` will not be able to achieve its maximum performance with them.
#'
#' It is recommended to use compilation options `-O3`, `-march=native`,
#' `-fno-math-errno`, `-fno-trapping-math`, and `-std=c99` or `-std=gnu99`.
#' These can be activated in multiple ways: \itemize{
#' \item (On Linux) Creating an empty text file `~/.R/Makevars` and adding this line there:
#' `CFLAGS += -O3 -march=native -fno-math-errno -fno-trapping-math`
#' (plus an empty line at the end), then installing the usual
#' way with `install.packages("cmfrec")`.
#' \item Installing `cmfrec` from source, but modifying the `Makevars` file (it has lines
#' that can be uncommented in order to enable these optimizations).
#' \item Modifying the global `Makeconf` variable. This is a file which defines the default
#' compilation options for \bold{all} R packages, so be careful about it. In Debian,
#' this file will typically be under `/etc/R/`, but this can vary in other operating systems.
#' In this file, replace all occurences of `-O2` with `-O3`, and all occurrences of
#' `-msse2` with `-march=native -fno-math-errno -fno-trapping-math` (e.g. open them
#' in some text editor or in RStudio and
#' use the 'Replace All' functionality) (not recommended to edit this global file, it should be
#' preferred to edit the local user Makevars instead).
#' }
#' This package relies heavily on BLAS and LAPACK functions, and benefits from SIMD vectorization.
#' For better performance, it is recommended to use an optimized backed such as MKL or OpenBLAS,
#' and to enable additional compiler optimizations that are not used by default in R.
#'
#' See this guide for details:
#' \href{https://github.com/david-cortes/installing-optimized-libraries}{installing optimized libraries}.
#' @param X The main matrix with interactions data to factorize (e.g. movie ratings by users,
#' bag-of-words representations of texts, etc.). The package is built with
#' recommender systems in mind, and will assume that `X` is a matrix in which users
Expand Down
4 changes: 2 additions & 2 deletions cmfrec/wrapper_untyped.pxi
Original file line number Diff line number Diff line change
Expand Up @@ -558,11 +558,11 @@ cdef extern from "cmfrec.h":
# void py_set_threads(int) noexcept nogil
# int py_get_threads() noexcept nogil

cdef public void cy_printf(char *msg) noexcept nogil:
cdef public void cy_printf(const char *msg) noexcept nogil:
with gil:
python_printmsg(msg)
stdout.flush()
cdef public void cy_errprintf(char *msg) noexcept nogil:
cdef public void cy_errprintf(const char *msg) noexcept nogil:
with gil:
python_printerrmsg(msg)
stderr.flush()
Expand Down
42 changes: 6 additions & 36 deletions man/fit.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions setup.cfg

This file was deleted.

2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ def test_supports_clang_reassociate(self):
setup(
name = "cmfrec",
packages = ["cmfrec"],
version = '3.5.1-6',
version = '3.5.1-7',
description = 'Collective matrix factorization',
author = 'David Cortes',
url = 'https://github.com/david-cortes/cmfrec',
Expand Down
13 changes: 5 additions & 8 deletions src/cmfrec.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,12 @@ typedef void (*sig_t_)(int);
#endif
extern IMPORTED_FUN void PySys_WriteStdout(const char *fmt, ...);
extern IMPORTED_FUN void PySys_WriteStderr(const char *fmt, ...);
void python_printmsg(char *msg);
void python_printerrmsg(char *msg);
void python_printmsg(const char *msg);
void python_printerrmsg(const char *msg);
void py_printf(const char *fmt, ...);
void py_errprintf(void *ignored, const char *fmt, ...);
extern void cy_printf(char *msg);
extern void cy_errprintf(char *msg);
extern void cy_printf(const char *msg);
extern void cy_errprintf(const char *msg);
#define printf py_printf
#define fprintf py_errprintf
#define fflush(arg) {}
#elif defined(_FOR_R)
#include <Rconfig.h>
Expand All @@ -150,8 +148,7 @@ typedef void (*sig_t_)(int);
#include <R_ext/Visibility.h>
#define USE_DOUBLE
#define printf Rprintf
#define fprintf(f, message) REprintf(message)
#define fflush(f) R_FlushConsole()
#define fflush(arg) R_FlushConsole()
#elif defined(MKL_ILP64)
#include "mkl.h"
#endif
Expand Down
29 changes: 10 additions & 19 deletions src/collective.c
Original file line number Diff line number Diff line change
Expand Up @@ -3325,8 +3325,7 @@ int_t collective_factors_cold
)
{
if (NA_as_zero_U && u_bin_vec != NULL) {
fprintf(stderr, "Cannot use 'NA_as_zero_U' when there is 'u_bin'\n");
fflush(stderr);
print_err_msg("Cannot use 'NA_as_zero_U' when there is 'u_bin'\n");
return 2;
}
int_t retval = 0;
Expand Down Expand Up @@ -3585,13 +3584,11 @@ int_t collective_factors_warm
)
{
if (u_bin_vec != NULL && (NA_as_zero_X || NA_as_zero_U)) {
fprintf(stderr, "Cannot use 'NA_as_zero' when there is 'u_bin'\n");
fflush(stderr);
print_err_msg("Cannot use 'NA_as_zero' when there is 'u_bin'\n");
return 2;
}
if (u_bin_vec != NULL && add_implicit_features) {
fprintf(stderr, "Cannot use implicit features when there is 'u_bin'\n");
fflush(stderr);
print_err_msg("Cannot use implicit features when there is 'u_bin'\n");
return 2;
}

Expand Down Expand Up @@ -7310,27 +7307,24 @@ int_t fit_collective_explicit_als

if (k_user && U == NULL && nnz_U == 0) {
if (verbose)
fprintf(stderr, "Cannot pass 'k_user' without U data.\n");
print_err_msg("Cannot pass 'k_user' without U data.\n");
retval = 2;
}

if (k_item && II == NULL && nnz_I == 0) {
if (verbose)
fprintf(stderr, "Cannot pass 'k_item' without I data.\n");
print_err_msg("Cannot pass 'k_item' without I data.\n");
retval = 2;
}

if (k_main && Xfull == NULL && nnz == 0) {
if (verbose)
fprintf(stderr, "Cannot pass 'k_main' without X data.\n");
print_err_msg("Cannot pass 'k_main' without X data.\n");
retval = 2;
}

if (retval == 2)
{
if (verbose) {
fflush(stderr);
}
return retval;
}

Expand Down Expand Up @@ -9411,35 +9405,32 @@ int_t fit_collective_implicit_als
int_t retval = 0;
if (k_user && U == NULL && nnz_U == 0) {
if (verbose)
fprintf(stderr, "Cannot pass 'k_user' without U data.\n");
print_err_msg("Cannot pass 'k_user' without U data.\n");
retval = 2;
}

if (k_item && II == NULL && nnz_I == 0) {
if (verbose)
fprintf(stderr, "Cannot pass 'k_item' without I data.\n");
print_err_msg("Cannot pass 'k_item' without I data.\n");
retval = 2;
}

if (k_main && nnz == 0) {
if (verbose)
fprintf(stderr, "Cannot pass 'k_main' without X data.\n");
print_err_msg("Cannot pass 'k_main' without X data.\n");
retval = 2;
}

if ((U != NULL && NA_as_zero_U) ||
(II != NULL && NA_as_zero_I))
{
if (verbose)
fprintf(stderr, "Cannot pass 'NA_as_zero' with dense data.\n");
print_err_msg("Cannot pass 'NA_as_zero' with dense data.\n");
retval = 2;
}

if (retval == 2)
{
if (verbose) {
fflush(stderr);
}
return retval;
}

Expand Down
Loading

0 comments on commit 3ee06a0

Please sign in to comment.