batch_request: update request with spec from dependencies

[pattonw]

Not sure about this one. Does this just change the default behaviour of a.merge(b) to prioritize specs in b instead of a?

Now that I'm looking at it I think this looks more complicated than I would expect from a merge function. I think I would expect merge to prioritize specs in a, and union roi's if necessary. Not sure why we are currently prioritizing a_spec, unless b_spec is non_spatial, in which case we replace a_spec. It seems this is cleared up in your version, no special handling of non_spatial, except now merge always prioritizes b.

[abred]

hmm, I see your point, that that's what one would expect. However I think in this case it might make sense to prioritize b, maybe the naming would have to be changed for that. b_spec is upstream from a_spec, so one might expect that it is more precise/has newer information so it should update the values in a_spec, maybe only the values that are not None in b_spec.

[pattonw]

Not sure if I follow. Why is b_spec upstream from a_spec? Regardless of how merge is written, you can get either behaviour by writing c = a.merge(b) or c = b.merge(a), so I think the merge function should stick to doing what you would expect. Unfortunately I think "merge" is a bit of an unclear name since it doesn't make it obvious which side is favoured. Maybe renaming the function to "update" and treating it like a dictionary update would make most sense. a.update(b) seems clear that anything in a and b gets overwritten by b.

[abred]

in batch_filter.py:

if isinstance(dependencies, BatchRequest):
    upstream_request = request.merge(dependencies)

request (a_spec) is the downstream request and dependencies (b_spec) is the current request, their merge is send upstream.

[pattonw]

Oh I see. Maybe we can just flip it here with: upstream_request = dependencies.merge(request) or upstream_request = request.update(dependencies)

[abred]

Sounds like a good option. Still, one question is which values should be updated? All, all that are None in a_spec, all that are not None in b_spec

[pattonw]

I think it should go in priority 1) union 2) take from a 3) take from b where you just choose the earliest option possible. (1, 3, 2) if considering an update function. So in the update case, all keys that are non None in b will be updated.

[abred]

pushed a suggestion, but haven't tested it yet.

[pattonw]

Thanks Peter! Sorry it took me a while to get to this, but I finally got around to checking it out and writing some quick tests. Merged this into funkey:v1.2-dev. I removed the checks on voxel_size and non-spatial since there might be cases you want to update those fields as well. If doing so would invalidate your request, the gunpowder pipeline should say so automatically.