Improve compile_header function
#513
Conversation
| except KeyError: | ||
| exposure_time = None | ||
| logging.warning("No exposure time") | ||
| object_name = input("REQUIRED: Please input a name for the object: ") |
There was a problem hiding this comment.
I'm not a fan of the input function for several reasons, but here are two main points:
- Testing this function is now more complex- you have to simulate a variable number of user inputs and while there are surely methods in pytest to do so, it'll require more setup.
- The flow of this function is not at all consistent. If values exist, no inputs are needed, sometimes some inputs are needed but not others. Automating this, if you want to loop through a list, will become nearly impossible.
The original implementation, which I see you commented out around line 50 is better (though can be tweaked)- identify all missing keywords from your input dictionary and fail if any of them are missing.
There was a problem hiding this comment.
The target user of this function (at the moment) is a high school student or undergrad who is using the spectrum for a different purpose. I want a function which will hold their hand through putting together our highest priority header keywords.
- I have used the
input()function for what I consider the highest priority keywords. - For keywords which they can likely figure out, e.g., Instrument , I've also used the
input()function but labeled them as OPTIONAL. - for keywords which may have existed in the original header, I've just pulled them from the original header with no user interaction.
If you have another suggestion for how to do the hand holding, I'm open!
I also wasn't planning on doing too much testing in our CI...I was gonna let the students do the testing. As they find things which don't work, we can modify the function and add tests?
I also recognize that this function is not mission critical to SIMPLE -- the database will still work with heterogenous spectra files -- so I'm ok if this function is not to the same standard as our ingest code.
There was a problem hiding this comment.
all that being said, this could be a useful script in astrodb_utils at some point, especially since it's striving to be consistent with SpectrumDM 1.2. We could rename it compile_ivoa_spectrum_dm_header. And I'm working on doing the same thing for light curves at the minute. So maybe making it high quality code is a good idea....
There was a problem hiding this comment.
One thing that occurs to me is that we can split this into two (or more) functions:
- one function that takes a dictionary and compiles the header and
- one that validates and creates the dictionary
That later function can be one using the input() calls (and we can add helper functions if needed), but the compile header function is more streamlined and can be tested or automated more readily.
There was a problem hiding this comment.
I think two functions is a very good idea. I think I may also go ahead and develop this in astrodb_utils instead of here in SIMPLE.
| try: | ||
| title = original_header_dict["title"] # trim so header wraps nicely | ||
| header.set("TITLE", title, "Data set title") | ||
| except KeyError: | ||
| pass | ||
| logging.warning("No title") | ||
| title = None |
There was a problem hiding this comment.
As an alternative to so many try..except blocks, consider using something like:
title = original_header_dict.get("title")
if title is not None:
header.set("TITLE", title, "Data set title")The python dictionary's get method automatically provides a default value of None if the key is not present: https://docs.python.org/3.10/library/stdtypes.html#dict.get
Improvements to make the
compile_headerfunction more user friendly.