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

Allow an ndarray as Center in rvals #532

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Conversation

wlmb
Copy link
Contributor

@wlmb wlmb commented Jan 31, 2025

No description provided.

@coveralls
Copy link

coveralls commented Jan 31, 2025

Coverage Status

coverage: 66.4% (-0.7%) from 67.081%
when pulling f85c12c on wlmb:rvals
into a47768e on PDLPorters:master.

Copy link
Member

@mohawk2 mohawk2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks really good. Please can you make it a die or confess if too many elements are supplied? I'm keen to not allow errors to pass through and cause problems later.

@wlmb
Copy link
Contributor Author

wlmb commented Jan 31, 2025

Thanks, this looks really good. Please can you make it a die or confess if too many elements are supplied? I'm keen to not allow errors to pass through and cause problems later.

Done. Would it be better to 'use Carp' and 'croak' instead of 'die'?

@mohawk2
Copy link
Member

mohawk2 commented Jan 31, 2025

Thanks, this looks really good. Please can you make it a die or confess if too many elements are supplied? I'm keen to not allow errors to pass through and cause problems later.

Done. Would it be better to 'use Carp' and 'croak' instead of 'die'?

I've just taken a look, and because there's a use PDL::Core '', barf is already imported (and is used elsewhere in the file). Please use that (that should have been my first ask, lesson for both of us there :-).

@wlmb wlmb force-pushed the rvals branch 2 times, most recently from 12b7e4b to c589c80 Compare January 31, 2025 19:22
@wlmb
Copy link
Contributor Author

wlmb commented Jan 31, 2025

I added a check on the dimensions of the center (it should be 1D) and changed warn to die to barf. I used topdl to unify the cases of a reference or of an ndarray and then converted to a perl array in order not to change the rest of the code.

@mohawk2
Copy link
Member

mohawk2 commented Jan 31, 2025

Looks really good! One more nitpick: PDL::Core::topdl($opt{CENTRE}) should be PDL->topdl($opt{CENTRE}).

@mohawk2
Copy link
Member

mohawk2 commented Jan 31, 2025

By the way, you wouldn't happen to know about doing ANOVA with generalised linear modelling (GLM)? I've got one case that doesn't work in https://rt.cpan.org/Ticket/Display.html?id=97925, having fixed the other? The problem I'm having is the remaining case gives a singular matrix when effect-coded up, and I'm grinding through the textbook to see if there are known techniques to avoid such singularity. If not, no problem!

@wlmb
Copy link
Contributor Author

wlmb commented Jan 31, 2025

Looks really good! One more nitpick: PDL::Core::topdl($opt{CENTRE}) should be PDL->topdl($opt{CENTRE}).

Done.

@mohawk2 mohawk2 merged commit 26a3269 into PDLPorters:master Jan 31, 2025
78 of 80 checks passed
@wlmb
Copy link
Contributor Author

wlmb commented Jan 31, 2025

By the way, you wouldn't happen to know about doing ANOVA with generalised linear modelling (GLM)? I've got one case that doesn't work in https://rt.cpan.org/Ticket/Display.html?id=97925, having fixed the other? The problem I'm having is the remaining case gives a singular matrix when effect-coded up, and I'm grinding through the textbook to see if there are known techniques to avoid such singularity. If not, no problem!

Sorry. I'm ignorant about ANOVA. Just read a little bit and seems interesting. A wild guess is that SVD (singular value decomposition) might be useful : it allows solving, in a sense, some singular problems. Regards, Luis

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.

3 participants