Code formatting + style cleanup in sipnet, workflow, ERA5 #3407

infotroph · 2025-01-04T21:40:23Z

Description

Code style edits: None should change output, most are whitespace changes.

I was reading the code in write.configs.SIPNET and realized the uneven spacing around operators was slowing down my reading, and decided to run styler::style_pkg() on the whole sipnet and workflow packages while I was at it.

I used styler's default styling settings to align with tidyverse style. I have some quibbles (e.g. it really wants the closing paren of a multi-line function call to be on its own line where I'd prefer it immediately after the last parameter), but none big enough to bother overriding.

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 agree that PEcAn Project may distribute my contribution under any or all of
- the same license as the existing code,
- and/or the BSD 3-clause license.
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.

Sweetdevil144 · 2025-01-05T07:57:40Z

Maybe Fixes Styling Section of #3280

infotroph · 2025-01-05T21:28:41Z

@Sweetdevil144 This PR manually fixes the styling of a few files, but alas it doesn't implement the automated styling that #3280 wishes for. I'd still love to have that!

Sweetdevil144 · 2025-01-06T08:17:00Z

@infotroph I just finished creating a script that automated styling using the styler package. Although the script takes forever to run as it navigates in all folders in all directories (it's been 10 minutes and the script still formats) due to the enormous size of file reading. I'm thinking of caching the formatted files in a local file which stores pre formatted files as follows : Read files from added files for committing. Simply format the files which have been changed. May include newly created files.
This works because I'll format and add all previous formatted files in the current project in the cached file systems, so only the modified or new files from in commits will be acted upon. Note that this would work as I will add a new pre-commit rule so after each commit, all filled would be styled.

mdietze · 2025-01-06T10:46:17Z

Be aware that the last time someone tried to apply style rules to the entire PEcAn repo, there were a fairly large number of places where manual reversions to the original code were required. I'm not up for having to fight an automated stylizer on every PR (or even periodically). I also want to make sure anything that gets implemented doesn't increase the bar of frustration that new members of the team feel when trying to learn how to submit successful PRs (too much time is already spent making changes that are not in bits of code the individual touched)

infotroph · 2025-01-06T11:39:41Z

I'll say more about automated styling in #3280; For this thread let's focus on whether to merge these specific changes.

mdietze

found some of the whitespace reformatting reduced readability considerably, especially when arguments to functions are shifted to be less indented than the function itself. Existing formatting makes it much easier to see what function is being called and what the arguments to that function are.

mdietze · 2025-01-06T10:47:58Z

base/workflow/R/create_execute_test_xml.R

+      notes = "Test_Run",
+      userid = user_id,
+      username = "None",
+      dates = Sys.Date()


I find the updated version is harder to read because the elements in the list don't line up with the parenthesis

You're in good company -- I see hanging indents for function calls in many codebases and {lintr} accepts either style, however the tidyverse style guide is explicit about recommending against them for calls even though it accepts them for definitions. With the disclaimer that I conveniently like the single-indent style better anyway, I do see virtue in just following the style guide here.

Also a PEcAn specific point: A lot of our function calls have very long names and assign their results into deeply nested objects, so a hanging indent on something like some$list$sublist$item <- PEcAn.data.atmosphere::extract_thing_from_stuff(
is 75 characters (plus any nesting for the block it appears in) of whitespace before we even start writing arguments, so at my usual window widths many well-intentioned hanging indents tend to get mangled by linewrapping.

BTW, I think we agree it is important that all arguments get the same indentation. We have a lot of places we used to have nice alignments until we added namespaces to the call without adjusting the wrapped lines beneath them:

- somefun(a = foo, + somepkg::somefun(a = foo, b = bar, c = FALSE)

I'm happy to stipulate that these need to be fixed whatever style we're using.

mdietze · 2025-01-06T10:49:57Z

base/workflow/R/create_execute_test_xml.R

+      dbname = "bety",
+      driver = db_bety_driver,
+      write = FALSE
+    ),


same. I do like the closing parenthesis on it's own line, but not indented at the same level as the original variable

If I had my druthers I'd use single-indent but with the closing paren always on the same line as the last argument (like in hanging indent), but I seem to be alone one that one.

mdietze · 2025-01-07T15:04:56Z

base/workflow/R/create_execute_test_xml.R

+  if (model.info$model_name == "LPJ-GUESS") {
+    settings$run$inputs <- c(
+      settings$run$inputs,
+      list(soil = list(id = 1000000903))


Not sure what the aim or this overall function is, or where it's called from, but this sort of deeply-buried hard-coded hack is something we should really avoid. I know you didn't add this and are just reformatting it, but I wonder if we could take the opportunity to fix it? First, this bit won't work if we're moving away from BETY being required (and never would have worked on a machine lacking this input ID). Second, as we move towards more people passing in ensemble soil inputs, this is going to break things (especially if a soil input is already present, which the code never checks for)

👍 to seeing something and taking the chance to fix it, but let's do it in a separate PR -- "Here's a big pile of whitespace changes with a few behavior changes mixed in" is my least favorite diff to review. I've made an issue for this in #3410

base/workflow/R/do_conversions.R

mdietze · 2025-01-07T15:07:41Z

base/workflow/R/run.write.configs.R

+          settings$pfts[[i]]$posteriorid,
+          con, settings$host$name,
+          return.all = TRUE
+        )


I think some of this is diff getting confused (the pid <- ... is still there, just shows in the next diff chunk), but IDK why styler didn't move "Posterior" down a line to match. That's a ding on my trust in the "just run styler::style_file() and it'll be good enough" theory.

Figured it out below: styler accepts multiple unnamed arguments on the same line, and when the call has a mix of unnamed and named arguments the result seems to depend on the exact starting configuration.

mdietze · 2025-01-07T15:33:37Z

modules/data.atmosphere/R/ERA5_met_process.R

+        }) %>%
+        dplyr::bind_rows() %>%
+        as.list()))
+     ) {


I think this is another place to rewrite and pull the site_info assignment out to its own line. In this case we don't even need to create a new temporary variable to do it!

mdietze · 2025-01-07T15:33:44Z

modules/data.atmosphere/R/ERA5_met_process.R

+      host = settings$database$bety$host,
+      user = settings$database$bety$user,
+      password = settings$database$bety$password
+    )


mdietze · 2025-01-07T15:34:27Z

modules/data.atmosphere/R/ERA5_met_process.R

+    site_info <- list(
+      site_id = qry_results$id, site_name = qry_results$sitename, lat = qry_results$lat,
+      lon = qry_results$lon, time_zone = qry_results$time_zone
+    )


mdietze · 2025-01-07T15:34:58Z

modules/data.atmosphere/R/ERA5_met_process.R

+        outfolder = site_outFolder,
+        start_date = site$start_date,
+        end_date = site$end_date
+      )


mdietze · 2025-01-07T15:35:48Z

modules/data.atmosphere/R/extract_ERA5.R

+    years <- seq(
+      lubridate::year(start_date),
+      lubridate::year(end_date),
+      1


revert and fix the close parenthesis on the next line

infotroph · 2025-01-07T16:59:00Z

Thanks for the review, @mdietze. I'll leave followup on some specific points inline, but I think the basic takeaway is that the bog-standard default output from {styler} isn't acceptable enough to adopt as it is. I'll see if I can find settings that come closer, but if not we should close this rather than try to manually massage individual lines.

infotroph · 2025-01-07T19:54:37Z

base/workflow/R/run.write.configs.R

+          settings$pfts[[i]]$posteriorid,
+          con, settings$host$name,
+          return.all = TRUE
+        )


Figured it out below: styler accepts multiple unnamed arguments on the same line, and when the call has a mix of unnamed and named arguments the result seems to depend on the exact starting configuration.

infotroph · 2025-01-07T22:23:07Z

base/workflow/R/start_model_runs.R

  if (PEcAn.settings::is.MultiSettings(settings) ||
-      PEcAn.settings::is.Settings(settings)) {
+    PEcAn.settings::is.Settings(settings)) {


Yeah, this one surprises me! I agree this is a readability loss, and would have expected it to force the opening brace onto its own line like for single-spaced function definitions:

if ( PEcAn.settings::is.MultiSettings(settings) || PEcAn.settings::is.Settings(settings) ) { write <- settings$database$bety$write ... }

...which I don't like as much as the hanging indent but it at least clearly separates the test conditions from the conditionally executed block.

infotroph · 2025-01-07T22:33:58Z

models/sipnet/R/model2netcdf.SIPNET.R

-mergeNC <- function(
-    ##title<< Aggregate data in netCDF files
-  files ##<< character vector: names of the files to merge
-  , outfile ##<< character: path to save the results files to. 


I think this was me manually deleting these upon noting that the Roxygen tags are already there with [light rewordings of] all of these.

models/sipnet/R/model2netcdf.SIPNET.R

infotroph · 2025-01-07T22:35:16Z

models/sipnet/R/model2netcdf.SIPNET.R

+## description<<
+## This function aggregates time periods in netCDF files. Basically it is just a
+## wrapper around the respective cdo function.


Suggested change

## description<<

## This function aggregates time periods in netCDF files. Basically it is just a

## wrapper around the respective cdo function.

infotroph · 2025-01-07T22:36:17Z

models/sipnet/R/model2netcdf.SIPNET.R

@@ -1,60 +1,64 @@
 #' Merge multiple NetCDF files into one
-#' 
-#' @param files \code{character}. List of filepaths, which should lead to NetCDF files.
+#'


Suggested change

#'

#'

#' This function aggregates time periods in netCDF files.

#' Basically it is just a wrapper around the respective cdo function.

#'

infotroph added 4 commits January 3, 2025 21:55

move duplicate code out of both legs of if/else, line wrapping

582f9b5

ran styler on ERA5 fns

578c20c

run styler on all files in models/sipnet

04139f8

run styler on workflow pkg

6b95332

github-actions bot added Tests Modules Base Models labels Jan 4, 2025

infotroph mentioned this pull request Jan 6, 2025

Feature : Automated linting for files #3280

Open

Sweetdevil144 mentioned this pull request Jan 6, 2025

feat: Implement automated styling on .R files #3408

Open

12 tasks

mdietze reviewed Jan 7, 2025

View reviewed changes

This was referenced Jan 7, 2025

Eliminate model/ID-specific hacks in create_execute_test_xml (well, and everywhere) #3410

Open

investigate commented-out phenology in do_conversions #3411

Open

infotroph commented Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code formatting + style cleanup in sipnet, workflow, ERA5 #3407

Code formatting + style cleanup in sipnet, workflow, ERA5 #3407

infotroph commented Jan 4, 2025

Sweetdevil144 commented Jan 5, 2025

infotroph commented Jan 5, 2025

Sweetdevil144 commented Jan 6, 2025 •

edited

Loading

mdietze commented Jan 6, 2025

infotroph commented Jan 6, 2025

mdietze left a comment

mdietze Jan 6, 2025

infotroph Jan 7, 2025

mdietze Jan 6, 2025

infotroph Jan 7, 2025

mdietze Jan 7, 2025

infotroph Jan 7, 2025 •

edited

Loading

mdietze Jan 7, 2025

infotroph Jan 7, 2025

infotroph Jan 7, 2025

mdietze Jan 7, 2025

infotroph Jan 7, 2025

mdietze Jan 7, 2025

mdietze Jan 7, 2025

mdietze Jan 7, 2025

mdietze Jan 7, 2025

infotroph commented Jan 7, 2025

infotroph Jan 7, 2025

infotroph Jan 7, 2025

infotroph Jan 7, 2025

infotroph Jan 7, 2025

infotroph Jan 7, 2025

	## description<<
	## This function aggregates time periods in netCDF files. Basically it is just a
	## wrapper around the respective cdo function.

-#'
+#'
+#' This function aggregates time periods in netCDF files.
+#' Basically it is just a wrapper around the respective cdo function.
+#'

Code formatting + style cleanup in sipnet, workflow, ERA5 #3407

Are you sure you want to change the base?

Code formatting + style cleanup in sipnet, workflow, ERA5 #3407

Conversation

infotroph commented Jan 4, 2025

Description

Motivation and Context

Review Time Estimate

Types of changes

Checklist:

Sweetdevil144 commented Jan 5, 2025

infotroph commented Jan 5, 2025

Sweetdevil144 commented Jan 6, 2025 • edited Loading

mdietze commented Jan 6, 2025

infotroph commented Jan 6, 2025

mdietze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infotroph Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infotroph commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sweetdevil144 commented Jan 6, 2025 •

edited

Loading

infotroph Jan 7, 2025 •

edited

Loading