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

Using map to produce a tuple inside StackObservations #310

Closed
SamuelBrand1 opened this issue Jun 28, 2024 · 5 comments
Closed

Using map to produce a tuple inside StackObservations #310

SamuelBrand1 opened this issue Jun 28, 2024 · 5 comments
Labels
enhancement New feature or request EpiAware good first issue Good for newcomers help wanted Extra attention is needed

Comments

@SamuelBrand1
Copy link
Collaborator

          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 #296 (comment)

@SamuelBrand1 SamuelBrand1 added enhancement New feature or request EpiAware labels Jun 28, 2024
@SamuelBrand1
Copy link
Collaborator Author

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

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

@SamuelBrand1 SamuelBrand1 added the good first issue Good for newcomers label Jun 28, 2024
@seabbs
Copy link
Contributor

seabbs commented Jun 28, 2024

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

@SamuelBrand1
Copy link
Collaborator Author

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

@SamuelBrand1 SamuelBrand1 added the help wanted Extra attention is needed label Jun 28, 2024
@seabbs
Copy link
Contributor

seabbs commented Jun 28, 2024

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

@seabbs
Copy link
Contributor

seabbs commented Jul 17, 2024

Closed by #358

@seabbs seabbs closed this as completed Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request EpiAware good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants