Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ABBA-based analysis for chop-and-nod observations #221

Merged
merged 9 commits into from
Nov 22, 2024

Conversation

hapipapijuris
Copy link
Contributor

Add ABBA method code #220
Should I add a scan time lapse plot like the figure outputted using pswsc function?

@hapipapijuris hapipapijuris added the feature New feature or request label Nov 17, 2024
@hapipapijuris hapipapijuris self-assigned this Nov 17, 2024
@hapipapijuris
Copy link
Contributor Author

  • Clean up ABBA method code
  • delete unnecessary comment
  • incorporate "abba" into all and Fire() and func auto

Copy link
Member

@astropenguin astropenguin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for implementing this feature. Your code assumes that the string abba exists somewhere in an observation name but this is not correct. I think you should update pswsc function instead of making a new one.

Should I add a scan time lapse plot like the figure outputted using pswsc function?

For the reason above, we don't need to make a new plotting code but simply replace the current spectrum made by the ABAB method with that by the ABBA method.

@astropenguin
Copy link
Member

astropenguin commented Nov 18, 2024

Minor comments:

  1. Why is .data used for abba_cycle but not for abba_phase?
    abba_cycle = (scan_onoff.data * 2 + is_second_half - 1) // 4
    abba_phase = (scan_onoff * 2 + is_second_half - 1) % 4
  2. The variable name da_abba_select does not look reasonable because it does not select the data anymore.
  3. Please keep the plot format align with other functions. For example, the figure size of 6 x 4 inches is not common.
    fig, ax = plt.subplots(figsize=(6, 4))
  4. I think required_elements = {0, 1, 2, 3} should be defined as a constant outside the function with the name of something like ABBA_PHASES.

@astropenguin astropenguin changed the title #220 Add ABBA method code Implement ABBA-based analysis for chop-and-nod observations Nov 18, 2024
@hapipapijuris
Copy link
Contributor Author

  • incorporate ABBA-based analysis code into pswsc func
  • change name da_abba_select to spec
  • make ABBA_PHASE in constant
  • change scan_onoff to scan_onoff.data

@hapipapijuris
Copy link
Contributor Author

I will add changing nod pair code in this issue. So please do not close.

Copy link
Member

@astropenguin astropenguin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update. Here are minor comments:

  1. Please sort constants alphabetically and use a plural form for the ABBA phases.
  2. Please add subtract_per_abba_cycle above subtract_per_scan (i.e. sort alphabetically).
  3. Description of subtract_per_abba_cycle looks not correct.
  4. Please rename subtract_per_scan because now it is not used per scan but per ABBA phase.
  5. ABBA_PHASE == set(...) (L1177) certainly works but it is not a good coding practice: A comparison (a constant to be compared) should usually put on the right-hand side. In addition, .values does not seem necessary in this case.

@astropenguin
Copy link
Member

astropenguin commented Nov 19, 2024

Related to 5., it would be great to check out a coding style called "early returns" or "guard clauses". For example, subtract_per_abba_cycle following this style should become something like:

def subtract_per_abba_cycle(dems: xr.DataArray, /) -> xr.DataArray:
    if not set(np.unique(dems.abba_phase)) == ABBA_PHASES:
        return xr.DataArray(np.nan)

    return (
        dems
        .groupby("abba_phase")
        .map(subtract_per_abba_phase)
        .mean("abba_phase")
    )

@hapipapijuris
Copy link
Contributor Author

I changed following Taniguchi-san's comment and reformatted the code.

@hapipapijuris
Copy link
Contributor Author

change explanation of subtract_per_abba_cycle and data type scan_onoff.data -> scan_onoff

Copy link
Contributor

@ShinjiFujita ShinjiFujita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not find any problems.

@astropenguin
Copy link
Member

@ShinjiFujita san, thank you for your checking. I will merge the PR and make a new public release soon. By the way, my apologies for keeping your PR #219 still open... I will check it soon as well.

@astropenguin astropenguin merged commit e563568 into main Nov 22, 2024
4 checks passed
@astropenguin astropenguin deleted the #issue220 branch November 22, 2024 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants