Allow ImageFileCollections to take a list of file names #403
Conversation
|
@mirca Thank you for the PR. I have been working on the same issue but that stalled. So I am very happy you found the time to implement this 👍 Some problems I have encountered which are not adressed in this PR:
I'll add some additional comments inline. |
There was a problem hiding this comment.
Overall this looks good 👍
However some comments should be adressed and it still needs a Changelog entry (https://github.com/astropy/ccdproc/blob/master/CHANGES.rst).
| @@ -57,11 +62,15 @@ class ImageFileCollection(object): | |||
| Raised if keywords are set to a combination of '*' and any other | |||
| value. | |||
| """ | |||
| def __init__(self, location=None, keywords=None, info_file=None): | |||
| def __init__(self, location=None, filenames=None, keywords=None, | |||
There was a problem hiding this comment.
New arguments should be added at the end of the list of arguments to ensure backwards-compatibility.
| Path to directory containing FITS files. | ||
| Default is ``None``. | ||
|
|
||
| filenames: str, list of str, or None, optional |
There was a problem hiding this comment.
I haven't tested it but is it possible to provide a single str?
There was a problem hiding this comment.
Thanks! Now it is possible, but ImageFileCollection.files is kept as list.
| fn = ['filter_no_object_bias.fit', 'filter_object_light_foo.fit'] | ||
| img_collection = image_collection.ImageFileCollection( | ||
| location=triage_setup.test_dir, filenames=fn, keywords=['filter']) | ||
| assert img_collection.files is fn |
There was a problem hiding this comment.
is tests are prone to errors, wouldn't == suffice?
| img_collection = image_collection.ImageFileCollection( | ||
| location=triage_setup.test_dir, filenames=fn, keywords=['filter']) | ||
| assert img_collection.files is fn | ||
| assert img_collection.summary is img_collection.summary_info |
There was a problem hiding this comment.
This test is not needed because they are always equal. summary_info is just the deprecated property.
refresh method now uses filenames attribute
|
@MSeifert04 Thank you very much for your review :) For now, |
| self._files = [] | ||
| if location: | ||
| self._files = self._fits_files_in_directory() | ||
| if self._filenames: | ||
| if type(self._filenames) is str: |
There was a problem hiding this comment.
I think there is some convention that one compares types using isinstance: isinstance(self._filenames, six.string_types) (not sure if it's really called that 😄 )
There was a problem hiding this comment.
This should also fix the Python 2.7 errors (I think it got unicode instead of str there).
| @@ -271,7 +284,13 @@ def refresh(self): | |||
| """ | |||
| keywords = '*' if self._all_keywords else self.keywords | |||
| # Re-load list of files | |||
| self._files = self._fits_files_in_directory() | |||
| if self._filenames: | |||
There was a problem hiding this comment.
This seems exactly like the block in __init__. To ease maintenance could you create a helper-method or check if this block could be put into _fits_files_in_directory?
There was a problem hiding this comment.
@MSeifert04 I added a helper method called _get_files(), do you like this name? xD
@mirca I think having the option to specify files in the location is already a great addition 👍 Let's see what @crawfordsm says, since he proposed the enhancement in #374. |
|
@mwcraig thanks for your review :) |
|
This looks really great and it is a good addition-- thanks @mirca ! I'm merging this pull request as I think it is useful on its own, but it would be good to still add the ability to add files from different locations so for the time being I'll leave the issue open. |
Implements #374.
I added an optional parameter named
filenameswhich should contain the names of the FITS files in the directory which should be added to the collection. IffilenamesisNoneandlocationis not, then all FITS files inlocationare added to the collection.Appreciate any feedback on this.