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

add is_quitte() #74

Conversation

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

is_quitte() tests the minimal quitte "standard", that is:

  • data.frame class and
  • existence and class of model, scenario, region, variable, unit, period, and value columns.

@orichters
Copy link
Contributor

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

What is the difference between is_quitte and the old is.quitte? https://github.com/pik-piam/quitte/blob/master/R/is.quitte.R

is.quitte() requires a 'quitte' class attribute, and a bunch of columns to be of type factor, both of which are irrelevant for (most) quitte functions.

@fbenke-pik
Copy link
Contributor

fbenke-pik commented Oct 13, 2023

So i assume both helpers are needed in different contexts and should exist alongside?
If not: I'd expect this to replace is.quitte. If yes: You should reference the other function and briefly point out differences between the two in the method signatures of both functions, so that a user of library can easily tell whether to use is.quitte or is_quitte.

Copy link
Contributor

@orichters orichters left a comment

Choose a reason for hiding this comment

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

What is the difference between is_quitte and the old is.quitte? https://github.com/pik-piam/quitte/blob/master/R/is.quitte.R

is.quitte() requires a 'quitte' class attribute, and a bunch of columns to be of type factor, both of which are irrelevant for (most) quitte functions.

In piamInterfaces, we certainly have lots of of functions using levels that expect them to come as factors, but that is ok as the data goes through as.quitte before. I don't fully understand why a second "is quitte" function is useful and would probably have preferred something like is.quitte(strict = FALSE) or so to avoid having these 2 functions with basically the same name, but different behavior.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

In piamInterfaces, we certainly have lots of of functions using levels that expect them to come as factors

I was referring to functions within the quitte package. Other packages are free to do whatever they want. If you are bored some day and want to make that code more robust, have a look into quitte::unique_or_levels().

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

In the end I did not need that function now, so I will close this merge request. I might open it again should I actually need it.

@orichters
Copy link
Contributor

I was referring to functions within the quitte package. Other packages are free to do whatever they want. If you are bored some day and want to make that code more robust, have a look into quitte::unique_or_levels().

Good to know. But as.quitte guarantees factor columns, I think the code is as robust as needed ;)

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