add tiled_noise to seedgen #47
Conversation
There was a problem hiding this comment.
Yes, the idea behind having dr5 and act_mr3 have the same noise seed index was that act_mr3 would be a subset of dr5, and so any noise sim from the act_mr3 subset made with dr5 code would reproduce what we got for act_mr3. I hadn't anticipated that this would break with a new noise sim algorithm. We should change the index in soapack to 3 as you suggest and perhaps call soapack from actsims to get the noise indices. But can approve this for now.
Also dr5->dr6 (my bad), but I'll fix that later.
actsims/util.py
Outdated
|
|
||
| assert(dm.name in self.dmdict.keys()) | ||
| dm_idx = self.dmdict[dm.name] | ||
| qid_idx = dmint.get_all_dr5_qids().index(qid) |
There was a problem hiding this comment.
I worry a bit about how this index could change e.g. if we insert a new qid somewhere in the middle of the list of maps. Can't think of a good way around it though.
There was a problem hiding this comment.
Hm, that makes sense to me though. I don't think this would break with the new algorithm, since the "new-ness" is captured in the variable TILED_NOISE. I just thought it made sense organizationally to have dr5 be assigned a unique value, in the absence of any other reason. But this reason seems like a good one, so I will defer to what you think is most appropriate.
That's a good point on the qid. You could hash the qid string itself, which just locks you into the string representations of the arrays (I think we are locked in anyway). The only hard part here is how to hash the string. Built-in python hash() generates integers that are too large for numpy seeds. Maybe just truncate the integer to ~8 digits? We'd probably be ok on collisions?
|
Hi Mat, I made a couple changes that should help. The "qid_idx" is now sourced from an additional column in all_arrays_dr5.csv; please see that PR. Also, up to 2 qids can be passed to handle seeds for simulating the correlation of 2 arrays (no more than 2!). Will comment for now and merge once you're able to review the corresponding soapack PR. |
|
@zatkins2 let's merge this if you think it is up-to-date (confirm and I'll merge) |
|
Hey Mat, thanks for reminding me about this. I think it's slightly out of date with the corresponding PR in soapack, but I'm going to take a look today and probably have only minor modifications to make, will ping you! |
This branch adds a single method to the actsims.util _SeedTracker class, for tiled noise sims, as opposed to single-patch noise sims. Besides setting a seed for a tile based on newly relevant data like qid and tile index, it makes two main changes to the class:
self.TILED_NOISE = 6as an instance variable. This ensures all seeds generated byget_tiled_noise_seeddo not overlap with previous noise sims.'dr5':3toself.dmdict. This allows users to pass aDR5datamodel instance to any of the class methods. The value, 3, is unique, which I think makes sense. However, it does conflict with line 22 of soapack.interfaces: the dictnoise_seed_indices, which assignsdr5to the value 0 -- would argue for this to be changed to 3 as well (if it is referenced?)