Remove overloaded getindex #314

charleskawczynski · 2023-08-31T20:44:46Z

This PR removes the overloading of getindex on ClimaCore fields.

also closes #317 #312

We access the individual fields within the Field of Tuples using the following rules:
Previously, regardless of in a loop or not:

field[i] .= ....
@. field[i] = g(other field)

Now: outside of a loop:

field.:i .= ...

Now: inside of a loop:

field.:($i) .=
@. field.:($$i) =

We also need to sometimes access elements that are compute quantities, for example, before,

field[n_stem+n_leaf] .=

Now, we need to do something like:

i_end =n_stem+n_leaf # define intermediate quantity
field.:($i_end) .=

regardless of if we are in a loop or not.

kmdeck · 2023-08-31T21:01:35Z

We can merge this first #315 and then in this PR, after rebasing, add back in the Aqua piracy test.

src/standalone/Vegetation/Canopy.jl

kmdeck · 2023-09-01T20:33:52Z

@charleskawczynski could you review this? I cant tag you since it is your PR :)

src/standalone/Vegetation/Canopy.jl

kmdeck · 2023-09-05T20:40:06Z

src/standalone/Vegetation/Canopy.jl

@@ -483,9 +483,10 @@ function ClimaLSM.make_update_aux(
                    )
                ) / 2
        end
-        @. β = moisture_stress(ψ[n_stem + n_leaf] * ρ_l * grav, sc, pc)
-        # We update the fa[n_stem+n_leaf] element once we have computed transpiration, below
+        i_end = n_stem + n_leaf


likewise here, and Im not totally clear on the rules around broadcasting -> use $$ but this seemed to give what we want!

Same as above, I think it's fine to add an intermediate if it helps fix this issue.

Do we need to use two $$ if we have a broadcast inside of a loop because the broadcast and loop are each an expression, so we need to interpolate the index value twice? If we have a broadcast outside of a loop, do we use just one $?

If we broadcast outside of a loop with a known index (like 2), we dont need any dollar signs:

julia> @. ψ.:2 = PlantHydraulics.water_retention_curve( retention_model, PlantHydraulics.effective_saturation(ν, ϑ_l.:2), ν, S_s, ) Float64-valued Field: [203.938]

If we want to index at the result of an expression, it doesnt really work. I think we need to define the intermediate i_end = n_stem+n_leaf.

julia> @. ψ.:$(n_stem+n_leaf) = PlantHydraulics.water_retention_curve( retention_model, PlantHydraulics.effective_saturation(ν, ϑ_l.:$(n_stem+n_leaf)), ν, S_s, ) ERROR: syntax: "(n_stem + n_leaf)" is not a valid function argument name around REPL[118]:1

kmdeck · 2023-09-05T20:40:56Z

src/standalone/Vegetation/Canopy.jl

@@ -506,7 +507,7 @@ function ClimaLSM.make_update_aux(
            canopy_surface_fluxes(canopy.atmos, canopy, Y, p, t)
        transpiration .= canopy_transpiration
        # Transpiration is per unit ground area, not leaf area (mult by LAI)
-        fa[n_stem + n_leaf] .= PlantHydraulics.transpiration_per_ground_area(
+        fa.:($i_end) .= PlantHydraulics.transpiration_per_ground_area(


not totally sure why we needed the $ here, either, since this is not in a loop.

We can/should probably document how this is working (with the single vs double $), but it looks fine to me

I agree that we should document how the indexing works now, Ill add some comments!

I'm also confused about why we use a $ here - shouldn't it just be fa.:i_end?

Im not sure. the :i_end notation says to look for the field associated with the symbol i_end if I understand correctly, and so it doesnt work here.

julia> i_end = n_stem+n_leaf 11 julia> fa.:i_end ERROR: Invalid field name i_end for type NTuple{11, Float64}

This doesnt work either:

julia> fa.:($(n_stem+n_leaf)) ERROR: syntax: invalid syntax "fa.((n_stem + n_leaf))" around REPL[104]:1 Stacktrace: [1] top-level scope @ REPL[104]:1

The integer needs $ to be interpolated otherwise the symbol (used to call getproperty(fa, ::Symbol)) is :i_end

charleskawczynski

Looks good to me!

juliasloan25

Looks good! I just had a couple of questions about places that weren't clear to me

src/standalone/Vegetation/Canopy.jl

juliasloan25 · 2023-09-07T12:30:40Z

src/standalone/Vegetation/Canopy.jl

@@ -483,9 +483,10 @@ function ClimaLSM.make_update_aux(
                    )
                ) / 2
        end
-        @. β = moisture_stress(ψ[n_stem + n_leaf] * ρ_l * grav, sc, pc)
-        # We update the fa[n_stem+n_leaf] element once we have computed transpiration, below
+        i_end = n_stem + n_leaf


Do we need to use two $$ if we have a broadcast inside of a loop because the broadcast and loop are each an expression, so we need to interpolate the index value twice? If we have a broadcast outside of a loop, do we use just one $?

juliasloan25 · 2023-09-07T12:31:47Z

src/standalone/Vegetation/Canopy.jl

@@ -506,7 +507,7 @@ function ClimaLSM.make_update_aux(
            canopy_surface_fluxes(canopy.atmos, canopy, Y, p, t)
        transpiration .= canopy_transpiration
        # Transpiration is per unit ground area, not leaf area (mult by LAI)
-        fa[n_stem + n_leaf] .= PlantHydraulics.transpiration_per_ground_area(
+        fa.:($i_end) .= PlantHydraulics.transpiration_per_ground_area(


I'm also confused about why we use a $ here - shouldn't it just be fa.:i_end?

src/standalone/Vegetation/PlantHydraulics.jl

Remove setindex/getindex overloading Remove getindex overloading Remove getindex overloading wip, trouble in loops tests pass aqua piracy turned on address two compat updates from compathelper as well made beta use same notation added comments regarding usage of getproperty

kmdeck · 2023-09-07T17:43:59Z

bors r+

bors · 2023-09-07T19:04:15Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

314: Remove overloaded getindex r=kmdeck a=charleskawczynski This PR removes the overloading of `getindex` on ClimaCore fields. also closes #317 #312 We access the individual fields within the Field of Tuples using the following rules: Previously, regardless of in a loop or not: ``` field[i] .= .... `@.` field[i] = g(other field) ``` Now: outside of a loop: ``` field.:i .= ... ``` Now: inside of a loop: ``` field.:($i) .= `@.` field.:($$i) = ``` We also need to sometimes access elements that are compute quantities, for example, before, ``` field[n_stem+n_leaf] .= ``` Now, we need to do something like: ``` i_end =n_stem+n_leaf # define intermediate quantity field.:($i_end) .= ``` regardless of if we are in a loop or not. Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>

charleskawczynski requested a review from kmdeck August 31, 2023 20:44

kmdeck linked an issue Aug 31, 2023 that may be closed by this pull request

type piracy in getindex method #277

Closed

kmdeck force-pushed the ck/rm_gi branch from fd195f9 to fa98ea2 Compare September 1, 2023 16:48

kmdeck reviewed Sep 1, 2023

View reviewed changes

src/standalone/Vegetation/Canopy.jl Outdated Show resolved Hide resolved

kmdeck requested a review from juliasloan25 September 1, 2023 20:33

kmdeck reviewed Sep 5, 2023

View reviewed changes

src/standalone/Vegetation/Canopy.jl Show resolved Hide resolved

kmdeck reviewed Sep 5, 2023

View reviewed changes

charleskawczynski commented Sep 6, 2023

View reviewed changes

juliasloan25 reviewed Sep 7, 2023

View reviewed changes

kmdeck force-pushed the ck/rm_gi branch from 790eb39 to e641ca3 Compare September 7, 2023 17:14

bors bot merged commit add6e9d into main Sep 7, 2023
7 checks passed

bors bot deleted the ck/rm_gi branch September 7, 2023 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove overloaded getindex #314

Remove overloaded getindex #314

charleskawczynski commented Aug 31, 2023 •

edited by kmdeck

Loading

kmdeck commented Aug 31, 2023

kmdeck commented Sep 1, 2023

kmdeck Sep 5, 2023

charleskawczynski Sep 6, 2023

juliasloan25 Sep 7, 2023

kmdeck Sep 7, 2023

kmdeck Sep 5, 2023

charleskawczynski Sep 6, 2023 •

edited

Loading

kmdeck Sep 6, 2023

juliasloan25 Sep 7, 2023

kmdeck Sep 7, 2023

charleskawczynski Sep 7, 2023

charleskawczynski left a comment

juliasloan25 left a comment

juliasloan25 Sep 7, 2023

juliasloan25 Sep 7, 2023

kmdeck commented Sep 7, 2023

bors bot commented Sep 7, 2023

Remove overloaded getindex #314

Remove overloaded getindex #314

Conversation

charleskawczynski commented Aug 31, 2023 • edited by kmdeck Loading

kmdeck commented Aug 31, 2023

kmdeck commented Sep 1, 2023

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

charleskawczynski Sep 6, 2023 • 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

charleskawczynski left a comment

Choose a reason for hiding this comment

juliasloan25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmdeck commented Sep 7, 2023

bors bot commented Sep 7, 2023

charleskawczynski commented Aug 31, 2023 •

edited by kmdeck

Loading

charleskawczynski Sep 6, 2023 •

edited

Loading