search

pyOpenSci / software-submission

Submit your package for review by pyOpenSci here! If you have questions please post them here: https://pyopensci.discourse.group/
94 stars 35 forks source link

astrodata #181

Open teald opened 6 months ago

teald commented 6 months ago

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

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

Domain Specific

Community Partnerships

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

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

Technical checks

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

Publication Options

JOSS Checks - [ ] The package has an **obvious research application** according to JOSS's definition in their [submission requirements][JossSubmissionRequirements]. Be aware that completing the pyOpenSci review process **does not** guarantee acceptance to JOSS. Be sure to read their submission requirements (linked above) if you are interested in submitting to JOSS. - [ ] The package is not a "minor utility" as defined by JOSS's [submission requirements][JossSubmissionRequirements]: "Minor ‘utility’ packages, including ‘thin’ API clients, are not acceptable." pyOpenSci welcomes these packages under "Data Retrieval", but JOSS has slightly different criteria. - [ ] The package contains a `paper.md` matching [JOSS's requirements][JossPaperRequirements] with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: *Note: JOSS accepts our review as theirs. You will NOT need to go through another full review. JOSS will only review your paper.md file. Be sure to link to this pyOpenSci issue when a JOSS issue is opened for your package. Also be sure to tell the JOSS editor that this is a pyOpenSci reviewed package once you reach this step.*

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.

Confirm each of the following by checking the box.

Please fill out our survey

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.

Batalex commented 6 months ago

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.

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

Editor comments

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

Batalex commented 5 months ago

Hey there @teald,

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

hamogu commented 5 months ago

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.

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:

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

hamogu commented 5 months ago

@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.

hamogu commented 4 months ago

@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!

aaryapatil commented 4 months ago

@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.

mwcraig commented 4 months ago

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

mwcraig commented 4 months ago

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

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

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.)

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:

Functionality

For packages also submitting to JOSS

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:

Final approval (post-review)

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.
Issues/PRs opened as part of review: - https://github.com/GeminiDRSoftware/astrodata/issues/18 - https://github.com/GeminiDRSoftware/astrodata/issues/19 - https://github.com/GeminiDRSoftware/astrodata/pull/21 - https://github.com/GeminiDRSoftware/astrodata/issues/22 - https://github.com/GeminiDRSoftware/astrodata/issues/23 - https://github.com/GeminiDRSoftware/astrodata/issues/25 - https://github.com/GeminiDRSoftware/astrodata/issues/26 - https://github.com/GeminiDRSoftware/astrodata/issues/27 - https://github.com/GeminiDRSoftware/astrodata/issues/28 - https://github.com/GeminiDRSoftware/astrodata/issues/29
test error traceback ```python-traceback _____________________________ 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 = , len = 8192, buffer = 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 ```
aaryapatil commented 4 months ago

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

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

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.)

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:

Functionality

For packages also submitting to JOSS

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:

Final approval (post-review)

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:

Poetry test run
py312: SKIP ⚠ in 0.02 seconds
py310: SKIP ⚠ in 0.02 seconds
py311: OK ✔ in 2 minutes 36.38 seconds
py310: SKIP (0.02 seconds)
py311: OK (156.38=setup[12.41]+cmd[0.00,4.36,139.60] seconds)
py312: SKIP (0.02 seconds)
coverage: SKIP (0.02 seconds)
congratulations :) (156.44 seconds)
teald commented 4 months ago

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 :)

teald commented 1 month ago

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:

Issues raised as part of this review + [Issue #18](https://github.com/GeminiDRSoftware/astrodata/issues/18) + [Issue #19](https://github.com/GeminiDRSoftware/astrodata/issues/19) + [Issue #22](https://github.com/GeminiDRSoftware/astrodata/issues/22) + [Issue #23](https://github.com/GeminiDRSoftware/astrodata/issues/23) + [Issue #25](https://github.com/GeminiDRSoftware/astrodata/issues/25) + [Issue #26](https://github.com/GeminiDRSoftware/astrodata/issues/26) + [Issue #27](https://github.com/GeminiDRSoftware/astrodata/issues/27) + [Issue #28](https://github.com/GeminiDRSoftware/astrodata/issues/28) + [Issue #29](https://github.com/GeminiDRSoftware/astrodata/issues/29) + [Issue #33](https://github.com/GeminiDRSoftware/astrodata/issues/33)

Pull Requests

All PRs have been merged into the main code:

List of Pull Requests made by reviewers + [Pull Request #20](https://github.com/GeminiDRSoftware/astrodata/pull/20) + [Pull Request #21](https://github.com/GeminiDRSoftware/astrodata/pull/21)

Thank you for your contributions!

List of Pull Requests related to this review. + [Pull Request #24](https://github.com/GeminiDRSoftware/astrodata/pull/24) + [Pull Request #31](https://github.com/GeminiDRSoftware/astrodata/pull/31) + [Pull Request #32](https://github.com/GeminiDRSoftware/astrodata/pull/32) + [Pull Request #34](https://github.com/GeminiDRSoftware/astrodata/pull/34) + [Pull Request #38](https://github.com/GeminiDRSoftware/astrodata/pull/38) + [Pull Request #41](https://github.com/GeminiDRSoftware/astrodata/pull/41) + [Pull Request #46](https://github.com/GeminiDRSoftware/astrodata/pull/46) + [Pull Request #48](https://github.com/GeminiDRSoftware/astrodata/pull/48) + [Pull Request #50](https://github.com/GeminiDRSoftware/astrodata/pull/50) + [Pull Request #51](https://github.com/GeminiDRSoftware/astrodata/pull/51) + [Pull Request #53](https://github.com/GeminiDRSoftware/astrodata/pull/53)

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:

Code example working with astrodata and ccdproc ```python 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.

hamogu commented 1 month ago

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.

hamogu commented 1 week ago

@aaryapatil and @mwcraig : I know we all can get busy at times, but it would be really great if you could check if the changes and replies by @teald address your concerns. If you think that the package should be accepted now, please edit your response above to 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.

If for some reason, you can't get to check that in the next two weeks or so, please let me know, so that we can proceed with this review in some other way! Thanks for your help and apologies to @teald that I'm letting you wait this long!

mwcraig commented 1 week ago

@hamogu -- thanks for the reminder. I should be able to take a look no later than monday.

hamogu commented 3 days ago

Hi @mwcraig and @aaryapatil It would be really great if you find time to check if your review has been answered. I understand that everyone is busy, but you've already spend som much time on the initial review, this should be much faster. Please reach out to me directly if you can't do that at all for any reason. @teald wrote a detailed response two months ago and I don't want to let them hanging. Please let me know if there is anything I can do to move this along.

aaryapatil commented 1 day ago

I have checked that my review has been answered and have edited my response above. Thank you for the detailed response, @teald, and my sincere apologies for the delay, @hamogu. This unfortunately slipped through my attention.