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

Clean up formatting of spectra_generator.md and related .py files #354

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

troyraen
Copy link
Contributor

@troyraen troyraen commented Dec 3, 2024

Clean up formatting by:

  • Organize imports and remove unused
  • Run autopep8 with a max line length of 100
  • Manually wrap long lines of text in spectra_generator.md and comments in .py files

@troyraen
Copy link
Contributor Author

troyraen commented Dec 3, 2024

@bsipocz I ran autopep8 to clean up the formatting. Does it look reasonable to you, or do you have other suggestions? The biggest remaining issue that I see is that some lines are still quite long. I'll point to one below. I will wrap long lines of text/comments manually (about to do that now) but I'm not planning to wrap lines of code manually. I tried using the --aggressive flag but that made it look really ugly in some spots, at least to my eye.

Comment on lines -77 to +80
querystring = "select observation_id from hsa.v_active_observation join hsa.instrument using (instrument_oid) where contains(point('ICRS', hsa.v_active_observation.ra, hsa.v_active_observation.dec), circle('ICRS', "+str(search_coords.ra.deg)+", " + str(search_coords.dec.deg) +", " + str(search_radius_arcsec) +"))=1 and hsa.instrument.instrument_name='"+str(instrument_name)+"'"
querystring = "select observation_id from hsa.v_active_observation join hsa.instrument using (instrument_oid) where contains(point('ICRS', hsa.v_active_observation.ra, hsa.v_active_observation.dec), circle('ICRS', "+str(
search_coords.ra.deg)+", " + str(search_coords.dec.deg) + ", " + str(search_radius_arcsec) + "))=1 and hsa.instrument.instrument_name='"+str(instrument_name)+"'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's one example. Line 79 is still 232 characters long.

Copy link
Member

Choose a reason for hiding this comment

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

this is what you got with the auto formatted? I agree that it's still very ugly and not really an improvement compared to the one super long line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, just autopep8 so far, no manual adjustments.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Looks good to me, and an imrpovement. While I left some comments, I would suggest to go ahead and merge this sooner rather than later, so any later cleanups won't be lost in the noise of these trivial changes.

spectroscopy/code_src/herschel_functions.py Show resolved Hide resolved
Comment on lines -77 to +80
querystring = "select observation_id from hsa.v_active_observation join hsa.instrument using (instrument_oid) where contains(point('ICRS', hsa.v_active_observation.ra, hsa.v_active_observation.dec), circle('ICRS', "+str(search_coords.ra.deg)+", " + str(search_coords.dec.deg) +", " + str(search_radius_arcsec) +"))=1 and hsa.instrument.instrument_name='"+str(instrument_name)+"'"
querystring = "select observation_id from hsa.v_active_observation join hsa.instrument using (instrument_oid) where contains(point('ICRS', hsa.v_active_observation.ra, hsa.v_active_observation.dec), circle('ICRS', "+str(
search_coords.ra.deg)+", " + str(search_coords.dec.deg) + ", " + str(search_radius_arcsec) + "))=1 and hsa.instrument.instrument_name='"+str(instrument_name)+"'"
Copy link
Member

Choose a reason for hiding this comment

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

this is what you got with the auto formatted? I agree that it's still very ugly and not really an improvement compared to the one super long line.

#from astroquery.sdss import SDSS
#from astroquery.simbad import Simbad
#from astroquery.vizier import Vizier
# from astroquery.ipac.ned import Ned
Copy link
Member

Choose a reason for hiding this comment

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

we'll need to clean up all the commented-out unused code, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm waiting to hear back from the scientists on whether this is still WIP. If they're done with it, I'll remove the commented-out code.

return (df_spec)
Copy link
Member

Choose a reason for hiding this comment

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

not sure if we can clean all these up without using ruff or something similar. Just leaving the comment for the example

Suggested change
return (df_spec)
return df_spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take them out when fix the docstrings since I'll be checking the return objects then. This is much easier to do manually than, say, deciding where to break long lines of code.

# Or should we take the average instead??
if len(tab) > 0:
print("More than 1 entry found" , end="")
print("More than 1 entry found", end="")
if not COMBINESPEC:
print(" - pick the closest")
Copy link
Member

Choose a reason for hiding this comment

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

Beyond scope of this PR, but I noticed this:
in the lightcurves notebook/code we had a verbose option, so we don't get all the noise from these prints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the other spectroscopy modules have the verbose option. Not sure why it was left out here.

@bsipocz
Copy link
Member

bsipocz commented Dec 4, 2024

and we need a skip ci label, as there is no reason for run CI for these notebooks until they are actually opted into the rendering/CI

@troyraen
Copy link
Contributor Author

troyraen commented Dec 4, 2024

Thanks @bsipocz.

I'm merging this now. More PRs will follow to complete the tech review.

@troyraen troyraen merged commit 0db074f into main Dec 4, 2024
3 checks passed
@troyraen troyraen deleted the raen/lint/spectra_generator branch December 4, 2024 02:02
github-actions bot pushed a commit that referenced this pull request Dec 4, 2024
* Organize imports. Remove unused.

* Run autopep8 with --max-line-length=100

* Wrap long lines of text 0db074f
@troyraen troyraen added the use case: spectroscopy Spectroscopy use case label Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
use case: spectroscopy Spectroscopy use case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants