Submitting Author: D.J. Teal (@teald) All current maintainers: @teald, @chris-simpson, @jehturner Package Name: astrodata One-Line Description of Package: Common interface for astronomical data products. Repository Link: https://github.com/GeminiDRSoftware/astrodata Version submitted: 2.9.2 Editor: @hamogu Reviewer 1: @aaryapatil Reviewer 2: @mwcraig Archive: TBD JOSS DOI: TBD Version accepted: TBD Date accepted (month/day/year): TBD

Code of Conduct & Commitment to Maintain Package

• [x] I agree to abide by pyOpenSci's Code of Conduct during the review process and in maintaining my package after should it be accepted.
• [x] I have read and will commit to package maintenance after the review as per the pyOpenSci Policies Guidelines.

Description

astrodata is a package meant to facilitate developing common interfaces for astronomical data formats. Often, specific instruments and models will have different ways of storing their data, including metadata. astrodata offers a single interface that uses metadata to resolve these disparate file formats, while enabling common operations and values to share the same interface. The abstraction is meant to be conceptually simple and meaningful to scientists.

Previously, astrodata was a core module within the DRAGONS package, and has been used for the various instruments that exist at the Gemini Observatory. It has proved useful in consolidating differences in metadata and data formatting between instruments that produce FITS files, which is a common pattern in astronomical data handling. Alongside automating interface selection based on these differences, it also comes with helpful operators and methods out-of-the-box, by extending astropy's NDData class.

Scope

• Please indicate which category or categories. Check out our package scope page to learn more about our scope. (If you are unsure of which category you fit, we suggest you make a pre-submission inquiry):

  • [ ] Data retrieval
  • [x] Data extraction
  • [x] Data processing/munging
  • [ ] Data deposition
  • [ ] Data validation and testing
  • [ ] Data visualization[^1]
  • [ ] Workflow automation
  • [ ] Citation management and bibliometrics
  • [ ] Scientific software wrappers
  • [ ] Database interoperability

Domain Specific

• [ ] Geospatial
• [ ] Education

Community Partnerships

If your package is associated with an existing community please check below:

• [x] Astropy:My package adheres to Astropy community standards
• [ ] Pangeo: My package adheres to the Pangeo standards listed in the pyOpenSci peer review guidebook

[^1]: Please fill out a pre-submission inquiry before submitting a data visualization package.

• For all submissions, explain how the and why the package falls under the categories you indicated above. In your explanation, please address the following points (briefly, 1-2 sentences for each):

  • Who is the target audience and what are scientific applications of this package?

    Astronomers & astronomical software developers

  • Are there other Python packages that accomplish the same thing? If so, how does yours differ?

    There are no specific packages we are aware of. There is some overlap with the gwcs package and our wcs module, but we are planning to collaborate with that package for future development and to reconsolidate those overlaps. Astrodata offers an interface based on astropy.nddata.NDData, but allowing more than one instance to be mapped to the same file (e.g., to multiple sets of FITS extensions).

  • If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted:

    N/A

Technical checks

For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:

• [x] does not violate the Terms of Service of any service it interacts with.
• [x] uses an OSI approved license.
• [x] contains a README with instructions for installing the development version.
• [x] includes documentation with examples for all functions.
• [x] contains a tutorial with examples of its essential functions and uses.
• [x] has a test suite.
• [x] has continuous integration setup, such as GitHub Actions CircleCI, and/or others.

Publication Options

• [ ] Do you wish to automatically submit to the Journal of Open Source Software? If so:

Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?

This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.

• [x] Yes I am OK with reviewers submitting requested changes as issues to my repo. Reviewers will then link to the issues in their submitted review.

Confirm each of the following by checking the box.

• [x] I have read the author guide.
• [x] I expect to maintain this package for at least 2 years and can help find a replacement for the maintainer (team) if needed.

Please fill out our survey

• [x] Last but not least please fill out our pre-review survey. This helps us track submission and improve our peer review process. We will also ask our reviewers and editors to fill this out.

P.S. Have feedback/comments about our review process? Leave a comment here

Editor and Review Templates

The editor template can be found here.

The review template can be found here.

May 13 '24 23:05 teald

Editor in Chief checks

Hi there! Thank you for submitting your package for pyOpenSci review. Below are the basic checks that your package needs to pass to begin our review. If some of these are missing, we will ask you to work on them before the review process begins.

Please check our Python packaging guide for more information on the elements below.

• [x] Installation The package can be installed from a community repository such as PyPI (preferred), and/or a community channel on conda (e.g. conda-forge, bioconda).
  • [x] The package imports properly into a standard Python environment import package. The package installation does not install the dependencies
• [x] Fit The package meets criteria for fit and overlap.
• [x] Documentation The package has sufficient online documentation to allow us to evaluate package function and scope without installing the package. This includes:
  • [x] User-facing documentation that overviews how to install and start using the package.
  • [x] Short tutorials that help a user understand how to use the package and what it can do for them.
  • [x] API documentation (documentation for your code's functions, classes, methods and attributes): this includes clearly written docstrings with variables defined using a standard docstring format.
• [x] Core GitHub repository Files
  • [x] README The package has a README.md file with clear explanation of what the package does, instructions on how to install it, and a link to development instructions.
  • [x] Contributing File The package has a CONTRIBUTING.md file that details how to install and contribute to the package.
  • [x] Code of Conduct The package has a CODE_OF_CONDUCT.md file.
  • [x] License The package has an OSI approved license. Seems like a BSD3 derivative is ok.

NOTE: We prefer that you have development instructions in your documentation too.

• [x] Issue Submission Documentation All of the information is filled out in the YAML header of the issue (located at the top of the issue template).
• [x] Automated tests Package has a testing suite and is tested via a Continuous Integration service.
• [x] Repository The repository link resolves correctly.
• [x] Package overlap The package doesn't entirely overlap with the functionality of other packages that have already been submitted to pyOpenSci.
• [ ] Archive (JOSS only, may be post-review): The repository DOI resolves correctly.
• [ ] Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

• [ ] Initial onboarding survey was filled out We appreciate each maintainer of the package filling out this survey individually. :raised_hands: Thank you authors in advance for setting aside five to ten minutes to do this. It truly helps our organization. :raised_hands:

Editor comments

Nothing to say from the EiC initial perspective, it's a strong submission!

May 26 '24 19:05 Batalex

Hey there @teald,

@hamogu will take over this submission as the editor 🫶 You are in good hands, happy reviewing y'all!

Jun 04 '24 16:06 Batalex

Editor response to review:

Editor comments

:wave: Hi @aaryapatil and @mwcraig! Thank you for volunteering to review for pyOpenSci!

Please fill out our pre-review survey

Before beginning your review, please fill out our pre-review survey. This helps us improve all aspects of our review and better understand our community. No personal data will be shared from this survey - it will only be used in an aggregated format by our Executive Director to improve our processes and programs.

• [ ] reviewer 1 survey completed.
• [ ] reviewer 2 survey completed.
• [ ] reviewer 3 (if applicable)

The following resources will help you complete your review:

1. Here is the reviewers guide. This guide contains all of the steps and information needed to complete your review.
2. Here is the review template that you will need to fill out and submit here as a comment, once your review is complete.

Please get in touch with any questions or concerns! Your review is due: <Insert deadline DATE HERE>

Reviewers: @aaryapatil @mwcraig Due date: end of June

Jun 05 '24 01:06 hamogu

@teald : @aaryapatil @mwcraig have agreed to review your package! Thanks! Links to the review guide and the template to get them started are in the post above; and of course they might open issues (or PRs!) in your repro if they have suggestions. In the meantime, feel free to reach out to me with any questions you might have.

Jun 06 '24 17:06 hamogu

@aaryapatil @mwcraig : I remember you both told me that you would need a little longer than the "three weeks" that we usually allocate to the first round of review and with the holiday coming up in the US, I understand that you are probably busy with other things this week. However, if you can make a realistic estimate when you can find time to review, it would be great if you could post here, so that the package authors know where we are in the process. Thank you so much for agreeing to review this package!

Jul 02 '24 21:07 hamogu

@hamogu Thank you for the ping. I am working on the review now but won't have much time this week. Realistically, I should have my review done by next week. Hope that is okay!

UPDATE: The rest of the review will be completed by July 16.

Jul 03 '24 14:07 aaryapatil

@hamogu -- thanks for the reminder; I should be able to get it done by the end of the day on Monday, July 15.

Jul 03 '24 18:07 mwcraig

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

• [x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• [x] A statement of need clearly stating problems the software is designed to solve and its target audience in README.
• [x] Installation instructions: for the development version of the package and any non-standard dependencies in README.
• [x] Vignette(s) demonstrating major functionality that runs successfully locally.
• [x] Function Documentation: for all user-facing functions.
• [ ] 👉 Examples for all user-facing functions. 👈
  • I looked at the functions in the Common API for Users. None have a function in the docstring, but all but add_header_to_table are well described elsewhere in the documentation. The only mention of add_header_to_table was in the MEF documentation and doesn't include any explanation. I guess the main thing I'm confused about is whether add_header_to_table adds an empty header that the user then needs to fill or whether it adds already-existing data.
  • To be clear, I think the only case where this really needs to be addressed is add_header_to_table and it could be addressed by saying a little more in the documentation about it.
  • However, the current Examples documentation have no actual examples. That is an issue.
• [x] Community guidelines including contribution guidelines in the README or CONTRIBUTING. :tada: really love the flowchart! \tada
• [ ] 👉 Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a pyproject.toml file or elsewhere. 👈
  • Adding urls to pyproject.toml that points to the project home on github and to the documentation would be great. That would add a "Project links" section in pypi. See the astropy pyproject.toml for how to add the URLs

Readme file requirements The package meets the readme requirements below:

• [x] Package has a README.md file in the root directory.

The README should include, from top to bottom:

• [x] The package name
• [ ] Badges for:
  • [x] Continuous integration and test coverage,
  • [x] Docs building (if you have a documentation website),
  • [ ] 👉 A repostatus.org badge, 👈
  • [ ] 👉 Python versions supported, 👈
  • [x] Current package version (on PyPI / Conda).

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

• [x] Short description of package goals.
• [x] Package installation instructions
• [x] Any additional setup required to use the package (authentication tokens, etc.)
• [x] Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
  • [x] Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
• [x] Link to your documentation website.
• [ ] If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
• [ ] 👉 Citation information 👈 -- there is a CITATION.md, but consider adding a link to it in the README.md

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

• [x] Package documentation is clear and easy to find and use.
• [x] The need for the package is clear
• [x] All functions have documentation and associated examples for use
• [x] The package is easy to install

Functionality

• [x] Installation: Installation succeeds as documented.
• [x] Functionality: Any functional claims of the software been confirmed.
• [x] Performance: Any performance claims of the software been confirmed.
• [ ] Automated tests:
  • [ ] 👉 All tests pass on the reviewer's local machine for the package version submitted by the author. Ideally this should be a tagged version making it easy for reviewers to install. 👈
    • Three tests failed, but that may have eben because one of the downloads failed (I know gemini is having network issues today), ~~even in a run where there were no download errors that I saw~~ looks like these were due to connection failure, see traceback below:
      • tests/test_object_construction.py::test_append_array_to_root_no_name ERROR
      • tests/test_object_construction.py::test_append_array_to_root_with_name_sci ERROR
      • tests/test_object_construction.py::test_append_array_to_root_with_arbitrary_name ERROR
    • Clear instructions for how to run the tests would be great -- I did a copy/paste from the github action workflow and tox started doing its thing, but I would have liked to have restricted the tests to just python 3.11. tox tried to do several versions.
  • [x] Tests cover essential functions of the package and a reasonable range of inputs and conditions.
• [x] Continuous Integration: Has continuous integration setup (We suggest using Github actions but any CI platform is acceptable for review)
• [ ] Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines. A few notable highlights to look at:
  • [x] Package supports modern versions of Python and not End of life versions.
  • [x] Code format is standard throughout package and follows PEP 8 guidelines (CI tests for linting pass)
    • 👉 I don't see a lint test on the CI. I gather from some code comments that linting is done with pylint. Running pylint astrodata on the source generates a number of errors, but these are almost all complaints about code complexity rather than format. More explicit instructions for running the lint test would be helpful, but the formatting is fine.

For packages also submitting to JOSS

• [ ] The package has an obvious research application according to JOSS's definition in their submission requirements.

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

• [ ] A short summary describing the high-level functionality of the software
• [ ] Authors: A list of authors with their affiliations
• [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
• [ ] References: With DOIs for all those that have one (e.g. papers, datasets, software).

Final approval (post-review)

• [ ] The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing:

Review Comments

@teald this is ready for a look

**Thanks for submitting this! 🎉 ** I am really excited about the potential of this package to fill some holes in the ecosystem. There are some details below in the "folded" sections that should be straightforward to address. Here are the high-level comments (background: I'm one of the maintainers for astropy.nddata and the sole maintainer for ccdproc):

1. What is your conception of how this should interact with the rest of the ecosystem (thinking mostly about nddata and ccdproc here)?
2. Does it make sense to upstream any of this (like the arithmentic handling or allowing for any WCS, not just an astropy.wcs) to astropy.nddata? Or to ccdproc?
3. Does it make sense for ccdproc to depend on astrodata or try to integrate usage of astrodata into it? ccdproc has never had a good way of handling MEF files, which is faintly ridiculous (I'm the maintainer of ccdproc so I'm looking in the mirror rather throwing stones here).
4. My take is that astrodata provides a way to abstract images and metadata from the underlying way they are stored, which is something that none of the current tools that I'm aware of provide. It may very well not make sense to upstream any of this.
5. Would it be possible to provide a small example of how to develop a processing tool with astrodata that goes beyond just adding properties and tags? In otherwords, once I have done those things what does astrodata do for me? I'm not suggesting a full reduction pipeline here (DRAGONS does that) but something that shows a step or two of processing files using would be helpful.

_____________________________ ERROR at setup of test_append_array_to_root_with_arbitrary_name ______________________________

filename = 'N20160524S0119.fits', path = '/Users/mattcraig/development/astronomy/astrodata/.tox/py311/_test_cache'
sub_path = 'raw_files', env_var = 'ASTRODATA_TEST', cache = True, fail_on_error = True

    def download_from_archive(
        filename,
        path=None,
        sub_path="raw_files",
        env_var="ASTRODATA_TEST",
        cache=True,
        fail_on_error=True,
    ):
        """Download file from the archive and store it in the local cache.

        Parameters
        ----------
        filename : str
            The filename, e.g. N20160524S0119.fits

        path : str or os.PathLike or None
            Path to the cache directory. If None, the environment variable
            ASTRODATA_TEST is used. otherwise, the file is saved to:
            os.path.join(path, sub_path, filename)

        sub_path : str
            By default the file is stored at the root of the cache directory, but
            using ``path`` allows to specify a sub-directory.

        env_var: str
            Environment variable containing the path to the cache directory.

        cache : bool
            If False, the file is downloaded and replaced in the cache directory.

        fail_on_error : bool
            If True, raise an error if the download fails. If False, return None.

        Returns
        -------
        str
            Name of the cached file with the path added to it.
        """
        # Handle None sub_path
        if sub_path is None:
            sub_path = ""
            warnings.warn(
                "sub_path is None, so the file will be saved to the root of the "
                "cache directory. To suppress this warning, set sub_path to a "
                "valid path (e.g., empty string)."
            )

        # Check that the environment variable is a valid name.
        if not isinstance(env_var, str) or not env_var.isidentifier():
            raise ValueError(f"Environment variable name is not valid: {env_var}")

        # Find cache path and make sure it exists
        root_cache_path = os.getenv(env_var)

        if root_cache_path is None:
            if path is not None:
                root_cache_path = os.path.expanduser(path)

            else:
                root_cache_path = os.path.join(os.getcwd(), "_test_cache")
                warnings.warn(
                    f"Environment variable not set: {env_var}, writing "
                    f"to {root_cache_path}. To suppress this warning, set "
                    f"the environment variable {env_var} to the desired path "
                    f"for the testing cache."
                )

                # This is cleaned up once the program finishes.
                os.environ[env_var] = str(root_cache_path)

        root_cache_path = os.path.expanduser(root_cache_path)

        if path is None:
            path = root_cache_path

        cache_path = os.path.join(os.path.expanduser(path), sub_path)

        if not os.path.exists(cache_path):
            os.makedirs(cache_path)

        # Now check if the local file exists and download if not
        try:
            local_path = os.path.join(cache_path, filename)
            url = GEMINI_ARCHIVE_URL + filename

            if cache and os.path.exists(local_path):
                # Use the cached file
                return local_path

>           tmp_path = download_file(url, cache=False)

astrodata/testing.py:479:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.tox/py311/lib/python3.11/site-packages/astropy/utils/data.py:1529: in download_file
    f_name = _download_file_from_source(
.tox/py311/lib/python3.11/site-packages/astropy/utils/data.py:1313: in _download_file_from_source
    with _try_url_open(
.tox/py311/lib/python3.11/site-packages/astropy/utils/data.py:1227: in _try_url_open
    return urlopener.open(req, timeout=timeout)
../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/urllib/request.py:519: in open
    response = self._open(req, data)
../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/urllib/request.py:536: in _open
    result = self._call_chain(self.handle_open, protocol, protocol +
../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/urllib/request.py:496: in _call_chain
    result = func(*args)
../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/urllib/request.py:1391: in https_open
    return self.do_open(http.client.HTTPSConnection, req,
../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/urllib/request.py:1352: in do_open
    r = h.getresponse()
../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/http/client.py:1395: in getresponse
    response.begin()
../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/http/client.py:325: in begin
    version, status, reason = self._read_status()
../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/http/client.py:286: in _read_status
    line = str(self.fp.readline(_MAXLINE + 1), "iso-8859-1")
../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/socket.py:706: in readinto
    return self._sock.recv_into(b)
../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/ssl.py:1314: in recv_into
    return self.read(nbytes, buffer)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <ssl.SSLSocket [closed] fd=-1, family=2, type=1, proto=0>, len = 8192, buffer = <memory at 0x139cdcdc0>

    def read(self, len=1024, buffer=None):
        """Read up to LEN bytes and return them.
        Return zero-length string on EOF."""

        self._checkClosed()
        if self._sslobj is None:
            raise ValueError("Read on closed or unwrapped SSL socket.")
        try:
            if buffer is not None:
>               return self._sslobj.read(len, buffer)
E               TimeoutError: The read operation timed out

../../../mambaforge/envs/astrodata-review-dev/lib/python3.11/ssl.py:1166: TimeoutError

The above exception was the direct cause of the following exception:

    @pytest.fixture
    def testfile2():
        """
        Pixels Extensions
        Index  Content                  Type              Dimensions     Format
        [ 0]   science                  NDAstroData       (4608, 1056)   uint16
        [ 1]   science                  NDAstroData       (4608, 1056)   uint16
        [ 2]   science                  NDAstroData       (4608, 1056)   uint16
        [ 3]   science                  NDAstroData       (4608, 1056)   uint16
        [ 4]   science                  NDAstroData       (4608, 1056)   uint16
        [ 5]   science                  NDAstroData       (4608, 1056)   uint16
        """
>       return download_from_archive("N20160524S0119.fits")

tests/test_object_construction.py:42:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

filename = 'N20160524S0119.fits', path = '/Users/mattcraig/development/astronomy/astrodata/.tox/py311/_test_cache'
sub_path = 'raw_files', env_var = 'ASTRODATA_TEST', cache = True, fail_on_error = True

    def download_from_archive(
        filename,
        path=None,
        sub_path="raw_files",
        env_var="ASTRODATA_TEST",
        cache=True,
        fail_on_error=True,
    ):
        """Download file from the archive and store it in the local cache.

        Parameters
        ----------
        filename : str
            The filename, e.g. N20160524S0119.fits

        path : str or os.PathLike or None
            Path to the cache directory. If None, the environment variable
            ASTRODATA_TEST is used. otherwise, the file is saved to:
            os.path.join(path, sub_path, filename)

        sub_path : str
            By default the file is stored at the root of the cache directory, but
            using ``path`` allows to specify a sub-directory.

        env_var: str
            Environment variable containing the path to the cache directory.

        cache : bool
            If False, the file is downloaded and replaced in the cache directory.

        fail_on_error : bool
            If True, raise an error if the download fails. If False, return None.

        Returns
        -------
        str
            Name of the cached file with the path added to it.
        """
        # Handle None sub_path
        if sub_path is None:
            sub_path = ""
            warnings.warn(
                "sub_path is None, so the file will be saved to the root of the "
                "cache directory. To suppress this warning, set sub_path to a "
                "valid path (e.g., empty string)."
            )

        # Check that the environment variable is a valid name.
        if not isinstance(env_var, str) or not env_var.isidentifier():
            raise ValueError(f"Environment variable name is not valid: {env_var}")

        # Find cache path and make sure it exists
        root_cache_path = os.getenv(env_var)

        if root_cache_path is None:
            if path is not None:
                root_cache_path = os.path.expanduser(path)

            else:
                root_cache_path = os.path.join(os.getcwd(), "_test_cache")
                warnings.warn(
                    f"Environment variable not set: {env_var}, writing "
                    f"to {root_cache_path}. To suppress this warning, set "
                    f"the environment variable {env_var} to the desired path "
                    f"for the testing cache."
                )

                # This is cleaned up once the program finishes.
                os.environ[env_var] = str(root_cache_path)

        root_cache_path = os.path.expanduser(root_cache_path)

        if path is None:
            path = root_cache_path

        cache_path = os.path.join(os.path.expanduser(path), sub_path)

        if not os.path.exists(cache_path):
            os.makedirs(cache_path)

        # Now check if the local file exists and download if not
        try:
            local_path = os.path.join(cache_path, filename)
            url = GEMINI_ARCHIVE_URL + filename

            if cache and os.path.exists(local_path):
                # Use the cached file
                return local_path

            tmp_path = download_file(url, cache=False)

            shutil.move(tmp_path, local_path)

            # `download_file` ignores Access Control List - fixing it
            os.chmod(local_path, 0o664)

        except Exception as err:
            if not fail_on_error:
                log.debug(f"Failed to download {filename} from the archive")
                log.debug(f" - Error: {err}")
                return None

>           raise IOError(
                f"Failed to download {filename} from the archive ({url})"
            ) from err
E           OSError: Failed to download N20160524S0119.fits from the archive (https://archive.gemini.edu/file/N20160524S0119.fits)

astrodata/testing.py:492: OSError

Jul 15 '24 22:07 mwcraig

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

• [x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• [x] A statement of need clearly stating problems the software is designed to solve and its target audience in README.
• [x] Installation instructions: for the development version of the package and any non-standard dependencies in README.
• [x] Vignette(s) demonstrating major functionality that runs successfully locally.
• [x] Function Documentation: for all user-facing functions.
• [ ] Examples for all user-facing functions.
  • ☝️The only available examples are those using Gemini, which requires installing DRAGONS and its dependencies. Filling in the User and/or Developer examples would be useful, especially with more examples of classes that inherit from astrodata. You could consider expanding the manual example here. Also refer to issue 33.
• [x] Community guidelines including contribution guidelines in the README or CONTRIBUTING.
• [ ] Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a pyproject.toml file or elsewhere.
  • ☝️ It would be good to include url(s) for the project in pyproject.toml. Refer to the packaging guide here.

Readme file requirements The package meets the readme requirements below:

• [x] Package has a README.md file in the root directory.

The README should include, from top to bottom:

• [x] The package name
• [ ] Badges for:
  • [x] Continuous integration and test coverage,
  • [x] Docs building (if you have a documentation website),
  • [ ] A repostatus.org badge,
    • ☝️ It would be useful to include this badge
  • [ ] Python versions supported,
    • ☝️ It would be useful to include this badge, especially for quick installations in environments.
  • [x] Current package version (on PyPI / Conda).

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

• [x] Short description of package goals.
• [x] Package installation instructions
• [x] Any additional setup required to use the package (authentication tokens, etc.)
• [x] Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
  • [x] Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
• [x] Link to your documentation website.
• [ ] If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
  • ☝️ For a first time reader going through the README, it might be worthwhile to expand upon the utility of astrodata beyond using astropy.io.fits alone. The current text only briefly mentions the extendability of astrodata.
• [ ] Citation information
  • ☝️ CITATION.md is present but hasn't been referenced in the README.md. The README.md is the first point of entry into the project GitHub, so adding a citation there is important.

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

• [x] Package documentation is clear and easy to find and use.
• [x] The need for the package is clear
• [x] All functions have documentation and associated examples for use
• [x] The package is easy to install

Functionality

• [x] Installation: Installation succeeds as documented.
  • ~~Remove "." following poetry install in quickstart~~ (fixed)
• [x] Functionality: Any functional claims of the software been confirmed.
• [x] Performance: Any performance claims of the software been confirmed.
• [ ] Automated tests:
  • [ ] All tests pass on the reviewer's local machine for the package version submitted by the author. Ideally this should be a tagged version making it easy for reviewers to install.
    • ☝️Running tests with poetry run tox -rp (as suggested in the quickstart) works well, but skips the coverage, probably because a specific version of python wasn't installed. Traceback is available below. It would be useful to add more information for testing a specific version of python.
  • [x] Tests cover essential functions of the package and a reasonable range of inputs and conditions.
• [x] Continuous Integration: Has continuous integration setup (We suggest using Github actions but any CI platform is acceptable for review)
• [x] Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines. A few notable highlights to look at:
  • [x] Package supports modern versions of Python and not End of life versions.
  • [x] Code format is standard throughout package and follows PEP 8 guidelines (CI tests for linting pass)

For packages also submitting to JOSS

• [ ] The package has an obvious research application according to JOSS's definition in their submission requirements.

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

• [ ] A short summary describing the high-level functionality of the software
• [ ] Authors: A list of authors with their affiliations
• [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
• [ ] References: With DOIs for all those that have one (e.g. papers, datasets, software).

Final approval (post-review)

• [ ] The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 10

Review Comments

Thank you for submitting this package @teald. I have completed my first pass, so you can go through it now. The package is already in great shape! If there are any questions, let me know. I will also be in touch in case something else comes up. I am looking forward to further using astrodata.

Pull requests opened as part of review:

• https://github.com/GeminiDRSoftware/astrodata/pull/20
• https://github.com/GeminiDRSoftware/astrodata/issues/33

Jul 15 '24 23:07 aaryapatil

Thank you both for your reviews! This feedback has been incredibly useful. I will continue working on these points and will respond to more general comments later this week or next week.

Update August 6: things have been taking a bit longer than anticipated. I'm hoping to have this wrapped up & a response ~this upcoming Monday (Aug. 12)

Update August 15: there have been some setbacks, I will just respond when this is finished. Hopefully soon 🤞 without further issues. Thanks for your patience!

Update September 6: We are waiting on feedback from a collaborator/contributor on the reviewer response, and should have it ready next week :)

Jul 22 '24 18:07 teald

Response

Hi all, I think I've addressed everything covered by the review. Thank you both for your PRs, issues, and notes here. They've helped immensely so far!

I'll itemize (for ease of reference) specific response points and how I've fixed them, then summarize other changes to the code.

Specific points (review checklist)

These are in order of reading the responses in the checklist above, with the ~sections in (parentheses).

1. (Documentation) Examples missing for user-facing functions
   1. All documentation in the "Common API for Users" now have examples, better explanations, or are deprecated.
   2. Specifically, add_header_to_table has been deprecated, as its functionality is no longer used. It will be removed in version 3.0.0.
   3. Covered in PR #50
   4. The previous examples directory was a vestigial directory, and has been removed.
2. (Documentation) URLs have been added to pyproject.toml
   1. Commit 8f74d89
3. (Documentation/README) Badges in README now have a repostatus and Python version badge
   1. Commit 6537a51
4. (Documentation/README) Citation information has been added to the README
   1. Commit b057511
5. (Functionality/Automated tests) Testing failures
   1. All testing failures encountered were due to intermittent service with the Gemini Archive (which is getting an upgrade!), as well as some issues with Actions and their runner configurations.
   2. Tracked in Issue #16 until updates complete
6. (Functionality/Packaging Instructions) Testing instructions unclear
   1. Developer documentation has been upgraded to reflect our new testing framework (using nox).
   2. Includes instructions for selecting specific tests, which tests are default, and specify python versions tested.
7. (Functionality/Packaging Guidelines) Repository is now linted via Actions
   1. Previously, relied on pre-commit alone.
   2. New lint.yml workflow just runs pre-commit to keep linting settings in pyproject.toml/.pre-commit-config.yaml
   3. Also, added nox -s initialize_pre_commit, automatically called by nox -s devshell and nox -s devconda to make it easier to set up the developer environment with pre-commit. This is still under testing.
8. (Documentation) Expanding on utility of astrodata in the README
   1. Include feature list and add references to manuals for futher reading alongside the Quickstart (Commit 246606d).

Issues

All issues raised as part of the initial review have been addressed:

Pull Requests

All PRs have been merged into the main code:

Thank you for your contributions!

Other changes of note

1. We've moved from using tox for testing to nox for testing and general automation. This was already planned for after this review to better support conda testing, but it made more sense to get this done now.
2. The developer documentation has been overhauled to describe several new development features motivated by these reviews:
   1. Automated development environment creation
   2. Developing without a virtual environment
   3. Clearer testing documentation

Responses to reviewer comments

mwcraig

1. What is your conception of how this should interact with the rest of the ecosystem (thinking mostly about nddata and ccdproc here)?

astrodata uses astropy.nddata for most of its core arithmetic functionality, including a number of the same, or slightly modified, mixins. There are likely some points of consolidation (see below) between the two that may happen. As for ccdproc, I think with CCDData already inheriting NDDataArray (which is similar to NDAstroData), there are opportunities for integration there. I would need to spend some more time thinking about that and looking at source, though.

Since astrodata provides support for MEF, as you pointed out in another question, as well as meta-data abstraction that is closely tied to the representation of the data compared to other tools, this streamlines the process of associating data and metadata from a user perspective. Users do not have to worry about managing the overhead that arises from handling lists of files and metadata tables. The descriptors provide a standardized way to create generic processing tools that hide boilerplate from the user, and tags allow for organization based on metadata properties or other traits of the data.

2. Does it make sense to upstream any of this (like the arithmetic handling or allowing for any WCS, not just an astropy.wcs) to astropy.nddata? Or to ccdproc?

There are plans to upstream some of the work done in the astrodata.wcs module to gwcs, specifically regarding the conversion between gwcs objects and their representation as FITS keywords, and then use gwcs throughout astrodata. But, that work hasn't been started yet and it's not clear when the resources will be available. I think that's probably the better avenue for more generic WCS support than astropy.nddata.

As mentioned above, I think there are some good opportunities for taking some of the handling done by astrodata and integrating it into, e.g., CCDData, where astrodata.AstroData objects naturally fit into the existing NDDataArray dependencies. I'm sure there are nuances there that would need to be sorted, but I could see CCDData/ccdproc using some of the features of astrodata to enhance their current functionality.

We could also consider how astrodata might assist/impact the goals of the closed APE 11. However, I think that'd require larger-scale coordination after this review has concluded (as part of APE PR #100).

3. Does it make sense for ccdproc to depend on astrodata or try to integrate usage of astrodata into it? ccdproc has never had a good way of handling MEF files, which is faintly ridiculous (I'm the maintainer of ccdproc so I'm looking in the mirror rather throwing stones here).

It depends on the goals of ccdproc moving forward. For MEFs it's feasible to break images up from a list into individual images ccdproc can then process. astrodata may, at the end of the day, be excessive for the work ccdproc does in its current scope.

4. My take is that astrodata provides a way to abstract images and metadata from the underlying way they are stored, which is something that none of the current tools that I'm aware of provide. It may very well not make sense to upstream any of this.

I think the biggest consideration is whether resources required to make these changes are worth the benefits themselves. There are natural places where astrodata's abstraction would help generalize/make other Astropy packages more flexible.

Right now, though, the work required to share data between, e.g., astrodata, astropy.nddata, and ccdproc is straightforward but requires some management between them that could be reduced to utility methods. I wrote up a quick, simple example to see how working with both astrodata and ccdproc went:

from astrodata import AstroData, create
from astropy.nddata import CCDData, NDData
from astropy.io import fits
import astropy.units as u
import numpy as np
import ccdproc

# Create a simple FITS file object with data and a header:
hdu = fits.PrimaryHDU(data=np.ones((100, 100)))
hdu.header["INSTRUME"] = "random_inst"
hdu.header["MODE"] = "random_mode"
hdu.header["UNIT"] = "adu"
hdu.header["EXPTIME"] = 5.0

# Create an AstroData object from the FITS file object:
ad = create(hdu)

# Access the underlying data and create a CCDData object:
ccd_image = CCDData(data=ad[0].data, unit=ad[0].hdr["UNIT"], meta=ad[0].hdr)

# Create a dark frame with the same shape as the data:
hdu_dark = fits.PrimaryHDU(data=np.random.random((100, 100)) * 10)
hdu_dark.header["INSTRUME"] = "random_inst"
hdu_dark.header["MODE"] = "random_dark_mode"
hdu_dark.header["UNIT"] = "adu"
hdu_dark.header["EXPTIME"] = 10.0

# Create an AstroData object from the FITS file object:
ad_dark = create(hdu_dark)

# Access the underlying data and create a CCDData object:
dark = CCDData(
    ad_dark[0].data,
    unit=ad_dark[0].hdr["UNIT"],
    meta=ad_dark[0].hdr,
)

# Subtract the dark frame from the data:
ccd_dark_subtracted = ccdproc.subtract_dark(
    ccd_image,
    dark,
    dark_exposure=dark.header["EXPTIME"] * u.s,
    data_exposure=ccd_image.header["EXPTIME"] * u.s,
)

This is a pretty minimal example, of course, but I think cases where the two interact can be managed primarily through accessing underlying data and metadata directly, rather than creating outright support for astrodata within the other packages.

5. Would it be possible to provide a small example of how to develop a processing tool with astrodata that goes beyond just adding properties and tags? In otherwords, once I have done those things what does astrodata do for me? I'm not suggesting a full reduction pipeline here (DRAGONS does that) but something that shows a step or two of processing files using would be helpful.

This is still being worked on. I'd like this example to be relatively complete in overview of how astrodata works, and I've been taking time coming up with something suitable. I think it's unavoidable to have it be somewhat complex, since astrodata and the properties and tags drive much of what users and developers will commonly interact with.

Development is being tracked in this issue. For now, the User Manual and Programmer's manuals have a bit more in-depth explanation, though I agree a more concrete, featureful example would be ideal.

aaryapatil

Thank you for your review and feedback! Let me know if you have any further questions or comments.

Sep 27 '24 21:09 teald

I see a very detailed reply from the package author thanks to @teald for explaining so detailed what work you have done to address the review!

@aaryapatil and @mwcraig: Please have a look and if you agree that that addresses all your concerns, please tick the box "The author has responded to my review and made changes to my satisfaction. I recommend approving this package." by editing the message of your review (and I appreciate a small note here, because GH will notify me if you post a new comment, but not if you edit an existing comment); if there is more to be done, please reply here so that we all know.

Sep 28 '24 00:09 hamogu