caracal-pipeline / stimela

Stimela 2.0
GNU General Public License v2.0
5 stars 3 forks source link

Should assign_based_on assignments be "weak"? #337

Open o-smirnov opened 5 days ago

o-smirnov commented 5 days ago

Again from TRON etc. If we have:

assign_based_on:
  obs:
    L2:
       ms: foo
       stokes: [I,Q,U,V]
       band: L
  band:
    L:
      stokes: [I]

we end up with stokes=[I], since the band section is evaluated after the obs section. This prevents us from making per-observation tweaks that override the per-band assignments. Arguably, this is the wrong way around!

A simple solution would be to define assign_based_on as being a "weak" assignment, i.e. have the assignment take effect only if not already assigned to previously. Thoughts?

JSKenyon commented 5 days ago

I think that the current order of operations makes more sense. If you do dict.update(x).update(y), you expect to have y not x. I think that this is a use-case for the command line. It has the highest precedence, correct? As a recurring theme, I am genuinely concerned about the stimela's booming complexity.

o-smirnov commented 5 days ago

If you do dict.update(x).update(y), you expect to have y not x. I

Yes, of course. The question is, should the semantics of assign_based_on be precisely like a dict update, or should its semantics be defined as a "weak update"? I don't think either case is more or less complex, it's just a question of settling on precisely what it should be doing. Current usage "in the wild" is compatible with both semantics, but I suspect the latter will be more useful going forward.

I think that this is a use-case for the command line. It has the highest precedence, correct?

Correct, and speaking of complexity, there's gymnastics in the code to ensure this, which would fall away if the assign sections had "weak update" semantics.

o-smirnov commented 5 days ago

I guess what I'm trying to get at, would not redefining of the meaning of assign in this way actually reduce complexity? Something to think about. I'm not entirely enamoured with how assign works right now.

o-smirnov commented 5 days ago

Got some test code on the branch for this, but may abandon this following our discussions. Kicking the can down the road for now.