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

Replace udunits2 with units #2989

Merged
merged 44 commits into from
Aug 1, 2022
Merged

Conversation

nanu1605
Copy link
Collaborator

Replace udunits2 with units package as udunits2 is an orphaned package.

@Aariq
Copy link
Collaborator

Aariq commented Jul 27, 2022

Looks like you need to run scripts/generate_dependencies.R

@nanu1605
Copy link
Collaborator Author

@Aariq, all the CI checks are passing so I think the replacement worked

Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

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

Woohoo! This is looking great.

base/db/R/insert.format.vars.R Outdated Show resolved Hide resolved
base/utils/R/ud_convert.R Outdated Show resolved Hide resolved
base/utils/R/ud_convert.R Outdated Show resolved Hide resolved
base/utils/R/ud_convert.R Outdated Show resolved Hide resolved
base/utils/R/ud_convert.R Outdated Show resolved Hide resolved
modules/data.atmosphere/R/merge.met.variable.R Outdated Show resolved Hide resolved
base/utils/R/ud_convert.R Outdated Show resolved Hide resolved
@nanu1605 nanu1605 requested a review from infotroph July 28, 2022 08:44
@nanu1605
Copy link
Collaborator Author

@infotroph, The changes you suggested are done. Please review it once

@Aariq
Copy link
Collaborator

Aariq commented Jul 28, 2022

Maybe some simple tests for the new functions would be a good idea? A couple correctness tests and maybe a test that the output is indeed numeric (and not class "units")

@infotroph
Copy link
Member

@nanu1605 This looks good, thank you. I support @Aariq's suggestion -- would you be willing to take a stab at some tests? I can help convert them into testthat format if you get stuck, but start by writing down the function calls you'd use for the correctness tests and how you'd decide the output is correct.

@nanu1605
Copy link
Collaborator Author

@infotroph @Aariq Sure I will try writing the tests for ud_convert

@mdietze
Copy link
Member

mdietze commented Jul 31, 2022

@infotroph does this PR pass your requested changes?

models/fates/R/met2model.FATES.R Outdated Show resolved Hide resolved
models/lpjguess/R/met2model.LPJGUESS.R Outdated Show resolved Hide resolved
Copy link
Member

@dlebauer dlebauer left a comment

Choose a reason for hiding this comment

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

Please update changelog

@nanu1605
Copy link
Collaborator Author

nanu1605 commented Aug 1, 2022

@dlebauer should i update CHANGELOG.md for this PR before merging?

@nanu1605 nanu1605 closed this Aug 1, 2022
@nanu1605 nanu1605 reopened this Aug 1, 2022
@nanu1605
Copy link
Collaborator Author

nanu1605 commented Aug 1, 2022

Sorry I closed it by mistake

@nanu1605 nanu1605 requested a review from dlebauer August 1, 2022 18:01
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Chris Black <[email protected]>
@mdietze mdietze dismissed stale reviews from infotroph and dlebauer August 1, 2022 19:07

Changelog updated

@mdietze mdietze merged commit 01b3704 into PecanProject:develop Aug 1, 2022
@nanu1605 nanu1605 deleted the replace_udunits2 branch August 1, 2022 19:10
@nanu1605 nanu1605 restored the replace_udunits2 branch August 2, 2022 13:56
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.

5 participants