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

standardize() fails in some cases #442

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

Conversation

strengejacke
Copy link
Member

Fixes #441

@etiennebacher
Copy link
Member

@strengejacke do you want to finish this one before 0.9.0? (I won't release before insight anyway)

@strengejacke
Copy link
Member Author

I can look at it. insight was just released on CRAN, so an update is actually not planned soon

@strengejacke
Copy link
Member Author

I can hardly remember the issue... I would add tests based on the example in the references issue, no?

@etiennebacher
Copy link
Member

Yes I think so, but based on @mattansb's comment in the related issue I'm not sure we should allow this

@mattansb
Copy link
Member

What issue are we trying to solve here?

I would argue that the examples in #441 should fail since it is not doing what one probably wants (to standardize the composite score).

@codecov

This comment was marked as outdated.

@strengejacke
Copy link
Member Author

Ok, this PR does not fully solve all issue mentioned in #441, but if I understand, this should also be intended? At least, it provides a small improvement, since it no longer fails for intercept-only models. If test coverage is sufficient, the small changes should not break any code, so we can merge this (or close this, if it seems to risky to address the edge cases, w/o knowing whether this change breaks something elsewhere).

@mattansb
Copy link
Member

I think it's too risky... I also seems maybe a little silly to do this for intercept only models - since the standardized intercept is always 0!

@strengejacke
Copy link
Member Author

@etiennebacher Seems like we - for now - don't plan to do anything regarding this issue, so I guess datawizard is ready for submission. We can keep this issue open to think about if we find a good solution how to deal with this (which can be: we already have the best solution, we add an informative message, ....).

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.

standardize() fails in some cases
3 participants