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

Add 2D Gaussian fitter to qlook commands #190

Merged
merged 11 commits into from
Aug 7, 2024
Merged

Add 2D Gaussian fitter to qlook commands #190

merged 11 commits into from
Aug 7, 2024

Conversation

ShinjiFujita
Copy link
Contributor

@ShinjiFujita ShinjiFujita commented Jul 29, 2024

Closes #187.

@astropenguin astropenguin changed the title Update qlook.py (issue187) Add 2D Gaussian fitter to fit module Aug 3, 2024
@astropenguin astropenguin changed the title Add 2D Gaussian fitter to fit module Add 2D Gaussian fitter to qlook commands Aug 3, 2024
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! I would like to ask you to fix the following things:

  • Package imports (L30-32): We put standard library under # standard library and external dependencies under # dependencies. Could you put import copy under # standard library (L14) and from scipy.optimize import curve_fit under from matplotlib.figure import Figure (L28)? Also, could you remove import pandas as pd as it is not used anymore in this module?
  • data[data != data] = 0.0 (L230, 489): Is it supposed to replace NaN values with zero? If so, could you consider to use data[np.isnan(data)] = 0 as it is more intuitive?
  • cont_fit = ... (L255, 514): The variable name cont_fit is a bit ambiguous because cont in this context sounds like not only continuum but also contours. Since this variable is not used throughout the function, could you simply remove this?
  • Code comments (L264-268, 523-527): Could you remove these lines? We manage the code changes by Git, so you do not need to leave the unused codes as comments.
  • If-else block (L269-288, 528-547): You do not need to do this because min_frequency and max_frequency are strings with explicit units. You can simply code like f"min_frequency: {min_frequency}, max_frequency: {max_frequency}".

@astropenguin astropenguin added the feature New feature or request label Aug 5, 2024
@astropenguin astropenguin added this to the v2024.8 milestone Aug 5, 2024
@astropenguin astropenguin self-requested a review August 6, 2024 11:20
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 fix! Could you please fix the two minor remaining things?

  • Could you put import copy just below # standard library (L14)? In general, import something should be placed above from module import something.
  • Could you remove cont_fit (L497)?

@astropenguin astropenguin self-requested a review August 7, 2024 15:51
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 so much! It looks good for me, so I will merge it into main.

@astropenguin astropenguin merged commit a221b33 into main Aug 7, 2024
4 checks passed
@astropenguin astropenguin deleted the issue187 branch August 7, 2024 15:53
@astropenguin astropenguin modified the milestones: v2024.8, v2024.9 Aug 7, 2024
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.

Add 2D Gaussian fitter to qlook commands
2 participants