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

Replaced transform function #3097

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

meetagrawal09
Copy link
Collaborator

Description

The aim of this PR is to replace the use of transform()
This PR fixes #2961

Motivation and Context

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@meetagrawal09
Copy link
Collaborator Author

meetagrawal09 commented Jan 27, 2023

Further occurrences of the transform function appear in pecan.models.maat and pecan.modules.priors
In these can we consider importing the dplyr package and using the mutate function to solve the issue ?

@Aariq any specific reason for doing the below for packages not importing dplyr ( anything affecting efficiency ? ):
Reference #2961comment

new_df <- old_df
new_df$new_col <- new_df$old_col * 2

Asking this because mutate() seems to be more intuitive while reading code

@Aariq
Copy link
Collaborator

Aariq commented Jan 28, 2023

@Aariq any specific reason for doing the below for packages not importing dplyr ( anything affecting efficiency ? ): Reference #2961comment

Just that it's sometimes a good idea to minimize the number of dependencies a package has.

@meetagrawal09
Copy link
Collaborator Author

meetagrawal09 commented Jan 29, 2023

All instances of transform() have been replaced
This PR is ready for a review

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.

This is looking very good! Is it ready for full review or are you still adding changes?

Edit: you answered that above! approving now.

@@ -59,19 +59,19 @@ convert.samples.MAAT <- function(trait.samples, runid) {
}
if ("Ha.vcmax" %in% names(trait.samples)) {
## Convert from kJ mol-1 to J mol-1
trait.samples <- transform(trait.samples, Ha.vcmax = PEcAn.utils::ud_convert(Ha.vcmax, "kJ", "J"))
trait.samples$Ha.vcmax <- PEcAn.utils::ud_convert(trait.samples$Ha.vcmax, "kJ", "J")
Copy link
Member

Choose a reason for hiding this comment

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

I'm amused that the transform's in this file were obviously intended to "save typing" but most of these lines are shorter without it!

modules/priors/vignettes/priors_demo.Rmd Outdated Show resolved Hide resolved
Co-authored-by: Chris Black <[email protected]>
@mdietze mdietze merged commit 336b18f into PecanProject:develop Jan 30, 2023
@meetagrawal09 meetagrawal09 deleted the replace-transform branch January 30, 2023 02:36
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.

Replace transform() with something programming-friendly
4 participants