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

Check if reporting Set fits gdx #6

Merged
merged 3 commits into from
May 30, 2024
Merged

Conversation

robinhasse
Copy link
Collaborator

The reporting template is now checked for consistency with the gdx. Surplus set elements trigger a warning and missing elements an error. This should help the user when selecting a reporting template that doesn't fit the run.

As bs and hs are currently just copies of bsr and hsr respectively, you always get a warning for the 0 elements. Not sure if we should change that such that or live with an expected warning. I could either implement an exception for the warning or the creation of aliases in the mapping.

Copy link
Contributor

@ricardarosemann ricardarosemann left a comment

Choose a reason for hiding this comment

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

Thank you for this feature, I hope this will be very helpful for debugging in the future.

If I'm not mistaken, this implementation would not directly catch the case I stumbled upon in the past and only give a warning (see inline comment). I would prefer this to be an error as well, as long as we believe that it will always fail further below.

R/convGDX2MIF.R Outdated
warning("The following set elements are mapped in the reporting template ",
"but not found in the gdx:\n ",
paste(capture.output(setElements[["surplus"]]), collapse = "\n "))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if this case should also throw an error? The case I had that confused me was this one, i.e. more set elements in the template than in the gdx. I haven't checked though if this will always lead to an error below and of course already the warning would help a lot with the debugging.

I tried to check whether some of your other changes might actually prevent this case from failing, but I don't think so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your are right, I was too lazy. In the reporting template, bs and hs are no longer defined as aliases of bsr and hsr respectively but as childs. As such, they can contain less elements and subsets than their parent set. This allows to make the test for inconsistencies more stringent. Any inconsistency (missing or surplus elements) between reporting template and gdx (missing or surplus elements) will lead to an error now.

Copy link
Contributor

@ricardarosemann ricardarosemann left a comment

Choose a reason for hiding this comment

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

Thank you for the change.

I found one spelling error, all else is good.

R/convGDX2MIF.R Outdated
@@ -27,6 +35,11 @@ convGDX2MIF <- function(gdx, tmpl = NULL, file = NULL, scenario = "default", t =
}

brickSets <- readBrickSets(tmpl)
inconsistencies <- .findInconsistenSetElements(brickSets, gdx)
if (!is.null(inconsistencies)) {
stop("The reporting template is not consisten with the gdx:\n ",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stop("The reporting template is not consisten with the gdx:\n ",
stop("The reporting template is not consistent with the gdx:\n ",

Spelling error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for spotting. I corrected that.

@robinhasse robinhasse merged commit 5bb270e into pik-piam:main May 30, 2024
2 checks passed
@robinhasse robinhasse deleted the robustTmpl branch May 30, 2024 09:02
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