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

notes/warnings fixes in data.atmosphere package #3218

Merged
merged 30 commits into from
Mar 12, 2024

Conversation

moki1202
Copy link
Collaborator

No description provided.

@moki1202 moki1202 marked this pull request as ready for review September 19, 2023 02:27
@moki1202 moki1202 marked this pull request as draft September 19, 2023 02:36
@moki1202 moki1202 marked this pull request as ready for review October 14, 2023 22:51
modules/data.atmosphere/R/debias_met_regression.R Outdated Show resolved Hide resolved
modules/data.atmosphere/R/debias_met_regression.R Outdated Show resolved Hide resolved
modules/data.atmosphere/R/debias_met_regression.R Outdated Show resolved Hide resolved
modules/data.atmosphere/R/met_functions.R Outdated Show resolved Hide resolved
modules/data.atmosphere/R/temporal.downscaling.R Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Models label Mar 9, 2024
Comment on lines +148 to +150
# TODO: Why do we divide by n?
# isn't precipitation_flux already an intensity?
precipitation_flux = rep(df$precipitation_flux / n, each = n),
Copy link
Member

Choose a reason for hiding this comment

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

The CF units on precipitation_flux are documented as kg m^-2 s^-1, so I would have expected it to be an average rate across the day and therefore to not need dividing by n. Is the division incorrect or am I missing something else? Is there a chance this input quantity was supposed to be named precipitation_amount (with units kg m^-2 = mm)?

Comment on lines +153 to +158
# TODO: Computation of solarR above already multiplies by resC2.
# Is multiplying it again here really correct?
# That's how the old data.table version did it
# (once when computing `solarR` and again when computing `SolarR`),
# so keeping it until proven wrong.
downwelling_photosynthetic_photon_flux = .data$solarR * light$resC2)
Copy link
Member

Choose a reason for hiding this comment

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

I don't know radiation calculations well enough to know if this is correct or not, just seemed odd to me especially since they're done in two separate steps. Can a met expert weigh in here, please?

Copy link
Member

@infotroph infotroph Mar 9, 2024

Choose a reason for hiding this comment

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

(see line 142 for the first multiplication by resC2)

Copy link
Member

@infotroph infotroph Mar 11, 2024

Choose a reason for hiding this comment

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

(And see deleted lines 136 and 138 for the same calculations as implemented before this PR)

@mdietze mdietze added this pull request to the merge queue Mar 11, 2024
Merged via the queue into PecanProject:develop with commit e4e14ae Mar 12, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants