CDCgov / Rt-without-renewal

https://cdcgov.github.io/Rt-without-renewal/
Apache License 2.0
17 stars 3 forks source link

Using `map` to produce a tuple inside StackObservations #310

Closed SamuelBrand1 closed 2 months ago

SamuelBrand1 commented 3 months ago
          This seems an unusual way to generate a collection? You're recursively generating an immutable `Tuple`. `map` is smart enough to produce a tuple give tuple input, I'm not sure why you've gone a different way here?

_Originally posted by @SamuelBrand1 in https://github.com/CDCgov/Rt-without-renewal/pull/296#discussion_r1650785638_

SamuelBrand1 commented 3 months ago

https://github.com/CDCgov/Rt-without-renewal/blob/1770c26a6e1af156e9ee3214c7f9c4762ec8dc4a/EpiAware/src/EpiObsModels/StackObservationModels.jl#L81-L86

I have a query about this bit because it:

  1. defines obs as an immutable Tuple
  2. Does as a seq of operations which create a new Tuple at each step.

I think it would be simpler to have a pattern like

obs = map(obs_model.models, obs_model.model_names) do model, model_name
@submodel obs_tmp = generate_observations(
            model, y_t[Symbol(model_name)], Y_t[Symbol(model_name)])
obs_tmp
end 

This might also be better for type stability and not generating lots of objects. map is smart enough to sidestep these issues e.g.

xs_tpl = Tuple(i for i = 1:10)
ys_tpl = Tuple(j for j = 1:10)

output = map(xs_tpl, ys_tpl) do x, y
  x^y
end

output isa Tuple
#true
seabbs commented 3 months ago

Yes I agree. I think for some reason I wasn't clear you could use map with a macro call inside of it

SamuelBrand1 commented 3 months ago

Yeah there might be an edge case problem... it would be a good first issue for someone to check out?

seabbs commented 3 months ago

Good idea. If this works it could be an option in ConcatLatentModels and CombineLatentModels vs the current use of self referencing functions

seabbs commented 2 months ago

Closed by #358