segrelab / cometspy

Python interface for running COMETS simulations and analyzing the results
GNU General Public License v3.0
11 stars 9 forks source link

Setting Initial Populations in a Loop #18

Closed hgscott closed 2 years ago

hgscott commented 2 years ago

I am getting an error when I try to add initial populations for a list of models in a for loop, but I only got the error on the second loop of the for loop, never the first.

I believe the error is coming from these lines: https://github.com/segrelab/cometspy/blob/cfab0856d3e92ad3acf4722465ef008026a42419/cometspy/layout.py#L1320-L1321

Line 1320 checks if the first element of the initial populations field is a list: in my case, m.initial_pop was a tuple, (15, 25, 1e-06), so the first element was an int. Line 1321 converts m.initial_pop from the tuple (15, 25, 1e-06) to a list with that whole tuple as the first element, [(15, 25, 1e-06)]. And from there everything runs fine.

But on a second loop, m.initial_pop[0] is still a tuple, so it is turned into [[(15, 25, 1e-06)]], so there is no second element for line 1325 to grab, giving me the out of bounds error.

hgscott commented 2 years ago

To fix this, I removed the [0] in line 1320, and I have opened pull request #19 with just that removed.

jeremymchacon commented 2 years ago

Hi @hgscott, thanks for helping out with COMETS! Would you mind showing your for loop? The reason that check is there is because initial_pop should end up as a list of lists, with each sublist being the location-specific pop size. In the case where a user is just doing a well-mixed sim and they do model.initial_pop = [0, 0, 1.e-5] (for example), that check turns this into a list of lists.

jeremymchacon commented 2 years ago

I agree that part of cometspy models is confusing though., which would be good to address. I think a better solution would be to have a function add_initial_pop(x, y, biomass) rather than modifying the list directly.

hgscott commented 2 years ago

Hi, @jeremymchacon- thanks for your speedy response!

You were right, I was not using a list when setting the initial pop (i.e. I was doing model.initial_pop = (0, 0, 1.e-5])), if I switch to a list it works fine! I will close my pull request.

I would definitely be happy to help make that part less confusing including working on a new function, just let me know whatever you think would be best.

jeremymchacon commented 2 years ago

Hi Helen, that would be great if you wanted to work on a function for this!

I imagine it would have a signature that looks like:

def add_initial_pop(x, y, biomass) which internally adds to the self.initial_pop list-of-lists each time. Then a second function could be

def remove_initial_pop() which resets self.initial_pop back to an empty list.