Skip to content

Move forcing script from fluxnet experiments to src #1238

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kmdeck
Copy link
Member

@kmdeck kmdeck commented Jul 17, 2025

Purpose

Attempts to clean up our fluxnet simulations. It mostly moves stuff into src but there is probably further cleanup to be had.

  • use the interpolating ability of TVI to handle gaps in data, rather than filling missing values by hand
  • remove metdrivers fluxnet script in favor of prescribed_forcing_fluxnet function in a new extension
  • set IC is a new function in the extension

It would be nice to pass dates or Itime to TVI but this does not currently work
It would be nice to be able to pass a preprocess function to TVI but this does not currently work, so I do unit changes by hand
Another cool thing would be if we could have TVI read a CSV and get the right column but Im not sure if that is worth it (rather than me reading in the csv and passing arrays to TVI)

To-do

Figure out how to handle to comparison to data in the output...
move plotting to LandSimulationVisualization extension

Content


  • I have read and checked the items on the review checklist.

Comment on lines -36 to +34
site_ID = ARGS[1]
site_ID = "US-MOz"
Copy link
Member

Choose a reason for hiding this comment

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

We should revert this before merging

@@ -53,8 +56,10 @@ ClimaTimeSteppers = "0.8"
ClimaUtilities = "0.1.24"
DataFrames = "1.4"
Dates = "1"
DelimitedFiles = "1"
Copy link
Member

Choose a reason for hiding this comment

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

I thought these packages would go under weakdeps rather than compat since they're used by the extension. I don't find very good Julia documentation about how to set up package extensions, but I did find this example repo someone made: https://github.com/pebeto/julia_extensions_example/tree/main

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to add a weak dependency with Pkg.add(name_of_package, target=:weakdeps).

Copy link
Member Author

Choose a reason for hiding this comment

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

^^ This is what I did (then package name and uuid is listed under weak deps). However the compat is separate from that.

We are not required to list compats for weak dependencies, but one of the Aqua tests checks that (we can disable it if we want). If we do want to list compats, though, there is not weak_compats - all compats are in a single list.

Our extensions have added compat in the past (Neural Snow - Flux, LandSimulationVisualization (very recently) - Poppler, Printf, etc)


[deps.ClimaLand.extensions]
FluxnetSimulations = ["DelimitedFiles", "Format"]
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to remove Format as a dependency? It seems strange to me to include a package that just handle string formatting?

Comment on lines +26 to +28
FluxnetSimulations =
Base.get_extension(ClimaLand, :FluxnetSimulations).FluxnetSimulations;

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if this work, but can you try

Suggested change
FluxnetSimulations =
Base.get_extension(ClimaLand, :FluxnetSimulations).FluxnetSimulations;
import ClimaLand.FluxnetSimulations as FluxnetSimulations

Comment on lines +3 to +5
function mask_data(t, v; val = -9999)
if ndims(v) == 1
not_missing_mask = .~var_missing.(v)
Copy link
Member

Choose a reason for hiding this comment

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

val isn't used in this function.

Comment on lines +1 to +2
var_missing(array; val = -9999) = array == val

Copy link
Member

Choose a reason for hiding this comment

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

Could documentation be added to all the functions in this file?

Comment on lines +51 to +52
Float64.(t),
Float64.(preprocess_func.(eachcol(v)...)),
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why this need to be Float64?

Comment on lines +64 to +67
if any(indices .== nothing)
nothing_id = findall(indices .== nothing)
@error("$(labels[nothing_id]) is missing in the data, but required.")
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if any(indices .== nothing)
nothing_id = findall(indices .== nothing)
@error("$(labels[nothing_id]) is missing in the data, but required.")
end
nothing_id = findall(indices .== nothing)
if !isempty(nothing_id)
@error("$(labels[nothing_id]) is missing in the data, but required.")
end

@@ -532,11 +500,11 @@ lines!(
label = "Ice, 1.25cm",
)

if drivers.SWC.status != absent
if comparison_data.SWC.absent == false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if comparison_data.SWC.absent == false
if !comparison_data.SWC.absent

@@ -599,11 +562,11 @@ lines!(
color = "blue",
)

if drivers.TS.status != absent
if comparison_data.TS.absent == false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if comparison_data.TS.absent == false
if !comparison_data.TS.absent

)
first_row = skip_header ? 2 : 1
idx = column_name_map[varname]
if idx isa Nothing
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if idx isa Nothing
if isnothing(idx)

Comment on lines +58 to +60
varnames = ("TA_F", "VPD_F", "PA_F", "P_F", "WS_F", "LW_IN_F", "SW_IN_F")
indices = [findfirst(columns .== varname) for varname in varnames]
column_name_map = Dict(zip(varnames, indices))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
varnames = ("TA_F", "VPD_F", "PA_F", "P_F", "WS_F", "LW_IN_F", "SW_IN_F")
indices = [findfirst(columns .== varname) for varname in varnames]
column_name_map = Dict(zip(varnames, indices))
varnames = ("TA_F", "VPD_F", "PA_F", "P_F", "WS_F", "LW_IN_F", "SW_IN_F")
colimn_name_map = Dict(varname => findfirst(columns .== varname) for varname in varnames)

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