search

traitecoevo / plant

Trait-Driven Models of Ecology and Evolution :evergreen_tree:
https://traitecoevo.github.io/plant
53 stars 20 forks source link

Patch is coupled to light environment #261

Closed rafaqz closed 3 years ago

rafaqz commented 4 years ago

I've written up the Canopy class for all canopy/light related environment things.

https://github.com/traitecoevo/plant/compare/develop...rafaqz:canopy

But we still have some coupling to light environment in patch I must have missed earlier:

https://github.com/traitecoevo/plant/blob/develop/inst/include/plant/patch.h#L211-L223

I'm not really across what this does! @dfalster do you know? I'm hoping we can also move that to Canopy as a method.

aornugent commented 4 years ago

Woah, the Canopy class is cool! I love the modularity it enables and the semantics of defining a model-specific environment (e.g. FF16) as one or more physical variables (e.g. canopy, water) are very intuitive.

rafaqz commented 4 years ago

Thanks, that's what I'm going for!

I'll do the same for water once this is working.

I'm not totally clear on wether the integrators for canopy and water could actually work separately if you have both canopy and water in one environment.

Maybe there are interactions between them. But we could then write something that handled that kind of interaction generically.

dfalster commented 4 years ago

I'm not really across what this does! @dfalster do you know? I'm hoping we can also move that to Canopy as a method.

Seems like it's for constructing environment with a given state. You should totally be able to move it into canopy as a method.

dfalster commented 4 years ago

Agree, I love the modularity. Heading in a great direction. Thanks Raf!

rafaqz commented 4 years ago

Great I moved it to Canopy.

But wish I understood propagation of const better... its very hard getting this to both compile and link when environment is const in compute_rates.

dfalster commented 4 years ago

In compute_rates we give access to the env so everyone can use it, but we don’t want anyone to change it. As far as I understand, putting const makes sure this is the case — I.e. env can only be accessed but not changed.

rafaqz commented 4 years ago

Yep I get that, and it makes sense. My problem is that const propagates and all the accessor methods also have to be const - so you don't have access to any non-const methods from the environment in compute_rates.

So you can't use environmentt.anymethodatall() because this (the Environment itself) is not const in those methods.

I just added a const double get_k_I(const FF16_Environment environment) method to FF16_Environment and it works. It just needs a FF16_Environement.cpp file now.

I don't know if it's the best way, my knowledge of const is not good enough really.

aornugent commented 4 years ago

I'm not really across what this does! @dfalster do you know? I'm hoping we can also move that to Canopy as a method.

Seems like it's for constructing environment with a given state. You should totally be able to move it into canopy as a methods.

Does this look like the piece we need for #248? If I understand the function correctly, it takes a vector of length 2n, with the first 1:n items corresponding to X and the remaining n+1:2n items corresponding to Y.

dfalster commented 4 years ago

Does this look like the piece we need for #248? If I understand the function correctly, it takes a vector of length 2n, with the first 1:n items corresponding to X and the remaining n+1:2n items corresponding to Y.

Could be!

dfalster commented 3 years ago

Closed via #262