another BCH review pass

[bhaller]

Hi @jiseonmin @petrelharp. Here's my next review pass. minimal.slim looks perfect to me now, so there are no changes there. The nonspatial_selection.slim and spatial_selection.slim scripts have a lot of changes; in fact, I wanted to completely redesign how they handle the sweep mutation, but I wasn't sure you guys would like those changes and I didn't want to cause too much chaos, so my redesigns are in new files, of the same names with a _BCH suffix added on. If you like my redesign, those files can replace the original files; if not, keep the original files (which also have lots of changes in them, but do not have that redesign work, so their diffs should be more manageable).

I'll probably wait for the fallout from this before I proceed further. Let me know when you've got all the changes (the ones you want, anyway) in main, and then I can do another review. I still haven't looked at the case_studies folder; I really want to get everything sorted out on these simpler models first before going to the more complex models, and in any case I don't have time to review the case studies right at this moment, so I wanted to get these diffs to you. (I do notice, though, that the monarchs model is named monarchs.slim, the mosquito model is mosquito.slim, and the pikas model is pikas.slim, but the toads model is CaneToads_RangeExpansion_Environment_Australia.slim; may I suggest toads.slim? :->)

[bhaller]

I was surprised by how many diffs I had for the nonspatial_selection.slim and spatial_selection.slim scripts, and some of my changes seemed familiar; perhaps my changes in a previous round of diffs didn't get merged in to main properly? Anyhow, I'm back on task, having checked two other things off my to-do list, and will now look at the case_studies models and commit diffs for them here; stay tuned. (I will not touch anything outside of the case_studies folder in this PR until I have gotten a thumbs-up from you guys, so you can work on the current diffs safely – I won't be changing them here.)

[bhaller]

Finished reviewing monarchs.slim. There were a lot of changes, so the diffs will probably be indecipherable, sorry. Not sure who will be responsible for managing my reviews of the case_studies models, but anyhow I'll ping @samchamper so he's in the loop.

[bhaller]

Finished reviewing mosquito.slim. The diffs on this are pretty straightforward. This one is yours, I think, @jiseonmin? I like the design of how it handles the map movement and the juvenile competition; it's really nice to see the new spatial modeling features getting used to such good effect! :->

[bhaller]

Finished pikas.slim. The diffs are pretty much all nits, except for a couple of comments suggesting possible improvements. @chriscrsmith this one is yours, IIRC?

[bhaller]

Finished the toads model. @silastittes you might want to look at these diffs. And with this, I think this code review pass is finished! @jiseonmin and @petrelharp, I believe this concludes my duties for the present moment. I'm happy to do one more review pass, pre-publication, but I think everybody else probably ought to integrate these changes first, and then Peter perhaps ought to do his own review pass. Everybody sees different things. :->

[bhaller]

Oh, one more comment for @chriscrsmith. For the annual climate variation, perhaps rather than just doing a random draw each year, it would be nice to model it as an autoregressive process or something? https://en.wikipedia.org/wiki/Autoregressive_model Might not be much more complicated than doing independent draws, and might alleviate some of the issue where an unusual draw in one year suddenly kills off a whole bunch of pikas; such events would instead tend to unfold across a series of years, perhaps fitting better to the El Niño / La Niña cycle, and might thus look more natural. Obviously this is not the core point of the model, but if it's easy to do, maybe it'd be nice? What do you think, @petrelharp?

[chriscrsmith]

Oh, one more comment for @chriscrsmith. For the annual climate variation, perhaps rather than just doing a random draw each year, it would be nice to model it as an autoregressive process or something? https://en.wikipedia.org/wiki/Autoregressive_model Might not be much more complicated than doing independent draws, and might alleviate some of the issue where an unusual draw in one year suddenly kills off a whole bunch of pikas; such events would instead tend to unfold across a series of years, perhaps fitting better to the El Niño / La Niña cycle, and might thus look more natural. Obviously this is not the core point of the model, but if it's easy to do, maybe it'd be nice? What do you think, @petrelharp?

I think it's a clever idea! Probably not too bad to implement like you said.

[bhaller]

No worries on closing it, @chriscrsmith. :-> This PR is not meant to be merged anyway, it's a place for review diffs that will get merged by hand, since you guys will agree with some of my changes and not others, etc. That's what @jiseonmin and I have been doing. So it doesn't actually matter whether the PR is "open" or not, in some sense. :-> But anyhow I see you got it re-opened.

[chriscrsmith]

Thanks @bhaller I have some work to do to address your suggestions, planning to get to it by the end of the week

[chriscrsmith]

Not matching an existing paper, just improvising here. I'm happy to work on getting better elevation data.

[chriscrsmith]

@bhaller I just updated the pikas.slim code to implement most of your smaller suggestions.

Before I change (1) the temperature fluctuations and (2) the elevation data, do you think the lead authors will be concerned about changing the pika section of the paper draft? Will need a new figure too.

[bhaller]

@bhaller I just updated the pikas.slim code to implement most of your smaller suggestions.

Before I change (1) the temperature fluctuations and (2) the elevation data, do you think the lead authors will be concerned about changing the pika section of the paper draft? Will need a new figure too.

I have no idea, really. I have kind of gotten the impression that this spatial_sims_standard repo is intended to be somewhat separate, and the models in it do not need to be identical to the models in the paper – can have somewhat different implementations, generate somewhat different output, etc. The ones here are polished versions for presentation, the ones in the paper repo are specifically to produce the figures in the paper. But I don't know how far that can be pushed :->. So I think it'd be a good topic to discuss in group meeting, maybe? @jiseonmin says she plans to look at this PR tomorrow once she's back in Eugene.

I guess my own two cents are that personally I think (1) I'd find it odd for the two versions of a given model to be out of sync very much between the two repos – beyond trivial differences like prettyprinting, variable names, that sort of thing; and (2) if you think the changes I suggested would make the model significantly better (pedagogically, aesthetically), then to me that would make it worthwhile for them to be reflected in the paper as well as in this repo. Not sure others would agree with either of those points, though. :->

[chriscrsmith]

right on! I'll keep moving forward with these changes

[bhaller]

right on! I'll keep moving forward with these changes

Maybe do them on an experimental branch or something, separate from the changes that you know are wanted?

[chriscrsmith]

Started another PR #27 . Not finished yet.