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.

Sign up for GitHub

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

Jump to bottom

Fixing some reported issues #280

Merged
merged 4 commits into from
Nov 11, 2023
Merged

Fixing some reported issues #280

merged 4 commits into from
Nov 11, 2023

Conversation

JTPetter
Copy link
Contributor

@JTPetter JTPetter commented Sep 25, 2023 •
edited
Loading

  • Fixes [Bug]: Graph variation components has not been built jasp-issues#2273: Fixed calculation of y-axis limits of variance component graph in gauge r&R. It sometimes did not include zero in the limits and then removed all bars.
  • Updated unit tests as graphs changed slightly after using jasp_theme_raw and geom_rangeframe instead of theme_jasp in variance component graph.
  • Fixes [Bug]: Percentage Process Variation Graph hadn't been built jasp-issues#2274: Fixed calculation of y-axis limits of percent process variation graph in gauge linearity. It sometimes did not include zero in the limits and then removed all bars.
  • Updated unit tests as graphs changed slightly after using jasp_theme_raw and geom_rangeframe instead of theme_jasp in bias and linearty graph and percent process variation graph.
  • Fixes [Bug]:  jasp-issues#2286: Tried to compare different data types and only aligned them when numeric. Now always aligning data type.

Based on #279 to make the unit tests work.

@JTPetter
Copy link
Contributor Author

Probably best to review and merge this PR after #279, will rebase as soon as it is merged

@JTPetter JTPetter requested a review from vandenman September 25, 2023 14:44
@JTPetter JTPetter force-pushed the fixingReportedIssues branch 2 times, most recently from 0bca182 to 42f1101 Compare October 2, 2023 09:54
@JTPetter
Copy link
Contributor Author

JTPetter commented Oct 2, 2023

Fixed another R4.3.1 issue that broke the reshaping from long to wide using a subgroup variable in process capability. From my side, this is ready to be reviewed now @vandenman

@JTPetter
Copy link
Contributor Author

JTPetter commented Nov 8, 2023

@vandenman I'm rebasing this now, could you try to review this before end of the week? This should definitely be in the upcoming release...

@JTPetter JTPetter force-pushed the fixingReportedIssues branch from 1a29d0a to 51f0d0b Compare November 8, 2023 00:53
vandenman
vandenman approved these changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@vandenman vandenman left a comment

Choose a reason for hiding this comment

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

Two minor comments, if you have time for them they would be nice to fix but otherwise, this can be merged.

R/msaGaugeRR.R Outdated Show resolved Hide resolved
R/msaAttribute.R
@@ -91,7 +91,8 @@ msaAttribute <- function(jaspResults, dataset, options, ...) {
jaspResults[["AAAtableGraphs"]] <- createJaspContainer(gettext("Attributes Agreement Analysis"))
jaspResults[["AAAtableGraphs"]]$position <- 16
}
jaspResults[["AAAtableGraphs"]] <- .aaaTableGraphs(ready = ready, dataset = dataset, measurements = measurements, parts = parts, operators = operators, options = options, standards = standards)
jaspResults[["AAAtableGraphs"]] <- .aaaTableGraphs(ready = ready, dataset = dataset, measurements = measurements,
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look quite right, but I guess this was already the case?
As in, two lines above you do

jaspResults[["AAAtableGraphs"]] <- createJaspContainer(gettext("Attributes Agreement Analysis"))

and here you do

jaspResults[["AAAtableGraphs"]] <- .aaaTableGraphs(...)

which means that you overwrite everything that was in jaspResults[["AAAtableGraphs"]] before. I mean, it probably works because it already worked before, but it's not very pretty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think there is quite a bit in the measurement systems analysis that is not pretty, I would keep this for now, as I have a bigger rewrite of this part planned for early next year...

@JTPetter @vandenman
Update R/msaGaugeRR.R
8464088 
Co-authored-by: Don van den Bergh <[email protected]>
@JTPetter JTPetter merged commit 5ab733b into jasp-stats:master Nov 11, 2023
2 of 5 checks passed
@JTPetter JTPetter deleted the fixingReportedIssues branch April 4, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vandenman vandenman vandenman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@JTPetter @vandenman