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

correctly set dimnames for matrices #428

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pachadotdev
Copy link
Contributor

@pachadotdev pachadotdev commented Dec 27, 2024

There was a bug in the matrix class such that attributes cannot be set (ref: https://stackoverflow.com/a/76019987/3720258). This was mentioned in #273.

With cpp11 0.5.1 I can add row and column names from C++ as long as the object is a sexp instead of a *_matrix<> (example: https://github.com/pachadotdev/economiccomplexity/blob/main/src/code.cpp#L28-L29)

With these changes, it is possible to assign colnames/rownames by borrowing from an input matrix or by setting dimnames = list(...).

Acknowledgements to @stephematician! It only took me 2 yrs to take intensive C++ courses at UofT to 1st learn proper C++, and then use that knowledge for my own research.

There is a problem with gh-actions. On my laptop, I can use auto, attribute_proxy<V> or `SEXP for lines 192-194 in matrix.hpp and it works. With GHA, it return an error. I fixed that with a workaround.

@pachadotdev pachadotdev changed the title fix #273 correctly set dimnames for matrices Dec 27, 2024
@stephematician
Copy link

You can test on similar platforms to github actions with devtools::check_win_release and devtools::check_rhub.

@pachadotdev
Copy link
Contributor Author

You can test on similar platforms to github actions with devtools::check_win_release and devtools::check_rhub.

yes! it's fixed now

@stephematician
Copy link

Nicely done!

Not that I'm a cpp11 developer - but as a matter of style, I would make the tests 'simpler' and more clear about what they test. For example, there's no need to calculate logarithms. I would test two things specifically:

  1. That you can assign dimnames given two (suitable) vectors in a list; e.g. a function with signature cpp11::doubles_matrix<> matrix_with_dimnames(int n, int m, cpp11::list dimnames) { /* ... */ }
  2. That you can assign dimnames using another matrices' dimnames, e.g. a function like cpp11::doubles_matrix<> duplicate_with_dimnames(cpp11::doubles_matrix<> x) { /* ... */ }

@pachadotdev
Copy link
Contributor Author

Nicely done!

Not that I'm a cpp11 developer - but as a matter of style, I would make the tests 'simpler' and more clear about what they test. For example, there's no need to calculate logarithms. I would test two things specifically:

1. That you can assign dimnames given two (suitable) vectors in a list; e.g. a function with signature `cpp11::doubles_matrix<> matrix_with_dimnames(int n, int m, cpp11::list dimnames) { /* ... */ }`

2. That you can assign dimnames using another matrices' dimnames, e.g. a function like `cpp11::doubles_matrix<> duplicate_with_dimnames(cpp11::doubles_matrix<> x) { /* ... */ }`

good point ! the log was just a "my brain defaults to log and OLS"

@pachadotdev
Copy link
Contributor Author

it would be good to merge this PR and then adapt the tests here

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.

2 participants