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

Echelle improvements #54

Merged
merged 51 commits into from
Feb 13, 2024
Merged

Echelle improvements #54

merged 51 commits into from
Feb 13, 2024

Conversation

emcangi
Copy link
Collaborator

@emcangi emcangi commented Jun 8, 2023

Pull Request Checklist

  • Code Follows PEP8 guidelines (explain any exceptions)
    Some lines are over length to avoid breaking in awkward positions

  • New routines don't duplicate existing functionality

  • Original authors of modified routines have been added as reviewers


Parameters
----------
file_index : index file (.npy)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the loaded index file is an array, not an .npy filename, so this might be confusing... I guess we could define a new datatype, but let's not if we can get away with it.

@planetarymike
Copy link
Collaborator

Thoughts about keeping the index generation routines in the echelle.py module for now, since we only plan to use this for echelle data?

@emcangi
Copy link
Collaborator Author

emcangi commented Jun 8, 2023

We could, I just figured it made sense to agnosticize it now. Would you prefer it that way?

@planetarymike
Copy link
Collaborator

Right now I think that the index generation routines are agnostic to the contents of the directories, but the echelle routines that look at the index to generate the reports and file pairing don't have any protections to make sure that the files in the index are only echelle files. Until we add protections in one or both places I think the safer option is to keep all of the index generation stuff in the echelle submodule.

@emcangi
Copy link
Collaborator Author

emcangi commented Jun 8, 2023

For now I just moved those functions back to the echelle file. What do you think?

@planetarymike
Copy link
Collaborator

I think to be consistent all of the functions that are specific to echelle and the file index need to be in echelle.py . Same for functions like get_countrate_diagnostics, which is really only useful for the echelle with the pixel ranges specified.

I think the geometry functions and the average pixel countrate functions might belong in geometry.py and statistics.py / integration.py ?

Some of the binning stuff could go into a new binning.py file? Or does another organization scheme make more sense to you?

@emcangi
Copy link
Collaborator Author

emcangi commented Jun 9, 2023

Yeah, I've been having difficulty deciding how to organize since a lot of the other functions are older, the module names very general ("instrument.py"), or the functions are specific to certain data levels (e.g. l1b).

As for the index, at this point the update_index function doesn't care what's in the folder, which doesn't seem to me to be directly tied to echelle, it's more of a folder/fits parsing function. If you insist it be part of echelle, I will put it there though. get_countrate_diagnostics (and related functions) should be in there though, those I forgot about. I'm also missing some docstring info actually, so I will add that.

Overall I thought it would be reasonable to upload these new functions now and then make a more concerted effort to organize next. My instinct would be to have l1a functions and l1b functions separate, but that will probably lead to duplicated code, so the right answer is probably to sort by topics ("binning", "line fitting", "geometry" etc) and then divide those files into sections if there need to be separate functions for l1b/l1a. But I can also organize them first instead.

@planetarymike
Copy link
Collaborator

planetarymike commented Jun 9, 2023 via email

@emcangi
Copy link
Collaborator Author

emcangi commented Jun 9, 2023

Hmmm, yeah, let's look at it next week.

emcangi added 6 commits August 8, 2023 13:56
… local destination for syncing files will cause rsync to silently fail, as well as related troubleshooting print statements (may be removed if desired).
…e improvements to classification of files as darks.
…ble names. Also created get_pix_range as its own function, as its functionality was needed in more than one place while developing echelle quicklooks.
…w also has a variety of extra arguments to allow greater control of plot attributes.
@planetarymike
Copy link
Collaborator

Any reason you prefer alphabetical order? A different order would probably be more readable, but alphabetical is probably better than what we have now.

https://stackoverflow.com/questions/10289461/what-is-a-good-way-to-order-methods-in-a-python-class

@emcangi
Copy link
Collaborator Author

emcangi commented Aug 9, 2023

Mostly because I don't know what other order would be logical, and through use of the routines, we become familiar with them and remember names of ones we use, so we can find them quickly.

Open to other ordering ideas though! Could also maybe include an index comment string at the top, but I don't know offhand if that's discourged in PEP8 or 257 or whatever they call the style now.

@planetarymike
Copy link
Collaborator

The link I shared has some decent suggestions; put the most important stuff first, followed by helper methods; group related routines by subject matter. We don’t need to be strict about this but alphabetical seems cumbersome to maintain…

@emcangi
Copy link
Collaborator Author

emcangi commented Aug 9, 2023 via email

…he slit on the detector image at Majd's request.
…it or date; encapsulated several functions relating to selecting files for which to run quicklooks (downselect_data), dark frames (get_dark_frames, subtract_darks), and coadding light frames (coadd_lights). Corrected an error in get_file_metadata's datetime field. Added some information back to the quicklook file per John Clarke's recommendations.
…top), added a bit of logic to allow for use with l1a or l1b files.
@emcangi emcangi changed the title Echelle Echelle improvements Aug 25, 2023
…utine

- Move get_grad_colors to graphics.py
- Move update_index routine from fits_processing.py to echelle.py
- Move binning-related functions from fits_processing.py to new binning.py
- Create a routine in binning.py to get the number of pixels per bin for given fits file
- Delete fits_processing.py; move contents to binning.py (new) or miscellaneous.py
- Add a routine to obtain data product level from filename
- Update imported modules in miscellaneous.py and __init__.py according to changes.
…eometry routines to echelle.py. Remove unused imports in geometry.py and add imported modules to support change to graphics.py. Echelle module changes handled in later commit due to extensive changes to that module.

Add idl_colorbars import to support geometry plotting routines.
…own hosts; delete commented-out code previously used for troubleshooting.
…r l1b data and l1a echelle data.

- Restore original functionality of detector_image
- Create second detector_image_echelle to support echelle quicklook creation
- Move plot code previously in detector_image to a lower-level routine, plot_detector, called by detector_image and detector_image_echelle
- Implement **kwargs for detector_image, detector_image_echelle
- Replace nan_color variable (previously deleted) with existing gray color
- Remove echelle-specific plotting choices, to be re-implemented in higher level echelle routines
- Delete imports no longer needed here as a result of these changes
…ion of separate echelle_graphics.py module.

- Move quicklook code to new module, echelle_graphics.py
- Modify coadd_lights to include necessary routines to perform dark subtraction and divide by total frames here rather than in the plot making code
- Update subtract_darks to maintain the same number of frames as light observation, but set nans if frames are bad
- Remove code handling orbit 9070 that was ported directly from IDL but not actually incorporated
- Improve get_dark_frames to handle case where both darks failed, and where an average is requested
- Move echelle slit start/end pixels to instrument.py
- Update module imports in echelle.py to account for above changes
…fleshing out docstrings for functions previously missing docstrings/accurate docstrings, removing old code that was commented out.
Copy link
Collaborator

@planetarymike planetarymike left a comment

Choose a reason for hiding this comment

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

Looks great--- I added some small comments, let me know what you think. Hoping code changes will be significantly less onerous in the future

"""

# Retrieve dark frames for subtraction
first_dark, second_dark = get_dark_frames(dark_fits)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think assuming that there is always a first and second dark will cause problems when processing the very earliest mission data, but we can deal with that later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now it throws an error if there's only 1, returns the 2 if there are 2, and returns 2 with the second being an average of 2:end if there's > 2. I can change the unpacking to a tuple at a later point though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Started on this with bcddfdf

maven_iuvs/echelle.py Outdated Show resolved Hide resolved
maven_iuvs/echelle.py Outdated Show resolved Hide resolved
maven_iuvs/download.py Show resolved Hide resolved
maven_iuvs/echelle.py Outdated Show resolved Hide resolved
maven_iuvs/echelle.py Outdated Show resolved Hide resolved
@@ -29,7 +29,7 @@ def __init__(self, filename):
self.__basename = _os.path.basename(filename)
self.__check_input_is_iuvs_data_filename()
hdulist = HDUList.fromfile(filename, mode='readonly',
memmap=True, save_backup=False,
memmap=False, save_backup=False, # Note: Changed memmap to False because having it True precluded making quicklooks. --EMC 8/10/23
Copy link
Collaborator

Choose a reason for hiding this comment

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

Happy to change all the loading routines to use fits.open() instead; IUVSFITS adds a lot of overhead processing time.

maven_iuvs/graphics/graphics.py Outdated Show resolved Hide resolved
maven_iuvs/graphics/line_fit_plot.py Outdated Show resolved Hide resolved
maven_iuvs/instrument.py Outdated Show resolved Hide resolved
…er adaptation to earlier in the mission when only 1 dark may have been present. Adjustments for the case of only one dark will be added later.
Move some non-top-level imports of astropy.io.fits to the top in integration.py;
Replace code in echelle.py and echelle_graphics.py that was using IUVSFITS class
…with new function that uses regular expressions.
@planetarymike
Copy link
Collaborator

still working on this? happy to review now or later

@emcangi
Copy link
Collaborator Author

emcangi commented Feb 13, 2024

I think I've addressed every comment, but it's hard to tell because of the way it displays. Take another final look?

@planetarymike
Copy link
Collaborator

LGTM, thanks for persevering through these changes!

Copy link
Collaborator

@planetarymike planetarymike left a comment

Choose a reason for hiding this comment

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

LGTM

@emcangi emcangi merged commit eb8a79c into lasp:master Feb 13, 2024
@emcangi emcangi deleted the echelle branch February 16, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Echelle quicklooks
2 participants