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

Fix create_derived_agasc_h5 to work for proseco_agasc #178

Merged
merged 3 commits into from
Jun 6, 2024

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jun 3, 2024

Description

create_derived_agasc_h5 was broken when trying to create a proseco_agasc file. The logic to check that all standard columns were available does not apply in this case.

Interface impacts

Adds new argument full_agasc to write_agasc(). If true (default), require exactly the standard AGASC column names and dtypes. If false, ensure that the output columns are a strict subset of the standard AGASC columns.

This also adds a new sys_args argument to main(). This helped out with debugging at one point and I left it in.

Testing

Unit tests

  • Mac
(ska3) ➜  agasc git:(fix-create-derived-h5) pytest                       
=================================================== test session starts ===================================================
platform darwin -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0
rootdir: /Users/aldcroft/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collected 75 items                                                                                                        

agasc/tests/test_agasc_1.py .....                                                                                   [  6%]
agasc/tests/test_agasc_2.py ..........sssss..........ss..................                                           [ 66%]
agasc/tests/test_agasc_healpix.py ...........                                                                       [ 81%]
agasc/tests/test_obs_status.py ..............                                                                       [100%]

============================================== 68 passed, 7 skipped in 7.92s ==============================================
(ska3) ➜  agasc git:(fix-create-derived-h5) git rev-parse HEAD                                             
abf213d98c2d5f66d614776b52dce8393cd50871

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

Created a miniagasc_1p7.h5 and proseco_agasc_1p7.h5 with the updated code and confirmed that the output files had identical data content.

python create_derived_agasc_h5.py miniagasc --version 1.7 --filter-faint --dec-order
python create_derived_agasc_h5.py proseco_agasc --version 1.7 --filter-faint --include-near-neighbors \\
  --proseco-columns --dec-order

miniagasc

In [2]: import tables
   ...: 
   ...: h5 = tables.open_file("/Users/aldcroft/ska/data/agasc/miniagasc_1p7.h5")
   ...: agasc_flt = h5.root.data[:]
   ...: h5.close()
   ...: with tables.open_file("miniagasc_1p7.h5") as h5:
   ...:     agasc_new = h5.root.data[:]
   ...: len(agasc_flt)
   ...: len(agasc_new)
   ...: np.all(agasc_flt == agasc_new)
   ...: 
Out[2]: True

proseco agasc

   ...: 
   ...: with tables.open_file("/Users/aldcroft/ska/data/agasc/proseco_agasc_1p7.h5") as h5:
   ...:     agasc_flt = h5.root.data[:]
   ...: with tables.open_file("proseco_agasc_1p7.h5") as h5:
   ...:     agasc_new = h5.root.data[:]
   ...: np.all(agasc_flt == agasc_new)
   ...: 
Out[1]: True

@taldcroft taldcroft requested a review from javierggt June 3, 2024 19:33
Copy link
Contributor

@javierggt javierggt left a comment

Choose a reason for hiding this comment

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

I did the following

  • checked the changes and they look OK to me.
  • ran unit tests and they pass
  • generated the proseco agasc (and verified that it fails in master

@taldcroft taldcroft merged commit f8d59f3 into master Jun 6, 2024
1 check passed
@taldcroft taldcroft deleted the fix-create-derived-h5 branch June 6, 2024 14:03
@javierggt javierggt mentioned this pull request Jul 30, 2024
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.

2 participants