feat(ShiftRA): add option to make shift 360-degree-periodic

[sjforeman]

Adds an option to ShiftRA to periodically wrap shifted RA coordinates, and the RA axis of any relevant dataset

[sjforeman]

@ljgray The black check is failing here, and I recently saw your (now closed) PR that noted the changes in the latest version of black. Is there a plan to apply the new version to the radiocosmology codes, or pin the black version in the CI, or something else?

[ljgray]

@ljgray The black check is failing here, and I recently saw your (now closed) PR that noted the changes in the latest version of black. Is there a plan to apply the new version to the radiocosmology codes, or pin the black version in the CI, or something else?

Ah, yeah I should have mentioned that somewhere. I think we settled on just having whoever opens a PR that fails due to black just update and run the newest version of black and include that as a commit in the PR (I think it's black 23.1 ). So I guess just run the newest version of black over everything here and include those changes in anther commit, and it'll get included when this is merged. I've already done this on ch_pipeline and ch_util

[sjforeman]

Ah, yeah I should have mentioned that somewhere. I think we settled on just having whoever opens a PR that fails due to black just update and run the newest version of black and include that as a commit in the PR (I think it's black 23.1 ). So I guess just run the newest version of black over everything here and include those changes in anther commit, and it'll get included when this is merged. I've already done this on ch_pipeline and ch_util

@ljgray Sounds good, thanks for clarifying!

[ljgray]

FYI, I merged in another PR that updated the black formatting, so there might be a merge conflict to deal with

[ljgray]

Overall I think the implementation is good. I wonder if we should separate the behaviour of wrapping the ra axis and actually applying the shift to the datasets into two separate flags, like periodic and apply_to_data. As far as I understand, the original reason for this task was to properly align the ra axis stamps with the underlying data, so modifying the underlying data has a slightly different effect than what was previously being done.

[sjforeman]

Overall I think the implementation is good. I wonder if we should separate the behaviour of wrapping the ra axis and actually applying the shift to the datasets into two separate flags, like periodic and apply_to_data. As far as I understand, the original reason for this task was to properly align the ra axis stamps with the underlying data, so modifying the underlying data has a slightly different effect than what was previously being done.

I'm not sure I understand the case you're describing, where you would want to shift and wrap the ra axis but not the data itself. Suppose the RA coordinates are accidentally 2deg behind the true RAs of each sample, and suppose the final sample in the axis is labelled as 359deg. You'd want to apply a +2deg shift to that, so it becomes 361deg. But, some tasks expect the RA axis to be monotonically increasing, so we'd like the periodic=True option to wrap the 361deg sample around to the beginning of the axis and relabel it as 1deg. In that case, I'd also expect that we want to wrap the datasets in the same way, such that the old RA=359deg data line up with the relabelled RA=1deg element.

[ljgray]

I'm not sure I understand the case you're describing, where you would want to shift and wrap the ra axis but not the data itself. Suppose the RA coordinates are accidentally 2deg behind the true RAs of each sample, and suppose the final sample in the axis is labelled as 359deg. You'd want to apply a +2deg shift to that, so it becomes 361deg. But, some tasks expect the RA axis to be monotonically increasing, so we'd like the periodic=True option to wrap the 361deg sample around to the beginning of the axis and relabel it as 1deg. In that case, I'd also expect that we want to wrap the datasets in the same way, such that the old RA=359deg data line up with the relabelled RA=1deg element.

Ah, yes I see what you mean