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

remove statements forcing kadi commands v1 #172

Merged
merged 2 commits into from
Apr 16, 2024
Merged

Conversation

javierggt
Copy link
Contributor

@javierggt javierggt commented Apr 3, 2024

Description

This removes statements forcing the use of kadi commands v1 in some top-level scripts.

The AGASC supplemet processing uses kadi.commands.get_observations and kadi.commands.get_starcats_as_table. Both are commands v2 functions, not present in v1. As a result, the statements removed in this PR do not affect anything within AGASC.

I ran the following command using this branch without error:

python -m agasc.scripts.update_mag_supplement --start 2021:011 --stop 2021:012

Also, weekly processing has already been running without issues for two week after moving away the commands v1 files.

Interface impacts

Testing

Unit tests

  • No unit tests
  • Mac
  • Linux
  • Windows

Independent check of unit tests by Jean

  • Linux
(ska3-masters) jeanconn-fido> pytest
================================================== test session starts ==================================================
platform linux -- Python 3.11.8, pytest-8.0.2, pluggy-1.4.0
rootdir: /proj/sot/ska/jeanproj/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collected 72 items                                                                                                      

agasc/tests/test_agasc_1.py .....                                                                                 [  6%]
agasc/tests/test_agasc_2.py ..........................................                                            [ 65%]
agasc/tests/test_agasc_healpix.py ...........                                                                     [ 80%]
agasc/tests/test_obs_status.py ..............                                                                     [100%]

============================================ 72 passed in 113.70s (0:01:53) =============================================
(ska3-masters) jeanconn-fido> git rev-parse HEAD
4811f585cb067968426daa5672180e3067813d86

Functional tests

No functional testing.

@taldcroft
Copy link
Member

@javierggt - This just needs a statement in the description reflecting what you said verbally today -- that you ran processing on HEAD with the flight code and it succeeded. Given that commands v1 is broken by virtue of changing the data file names, I think this is sufficient to demonstrate that the configuration lines being removed have no impact.

…on (#168)

* Improve logging to make it easier to re-run with the same configuration

* make sure the args log file is not overwritten

* run update_supplement.update according to args, regardless of agasc_ids in args file
@jeanconn jeanconn self-requested a review April 16, 2024 14:12
@jeanconn
Copy link
Contributor

#168 has already been reviewed, so I think the open PR is really just the other commit 9e1d5d9 .

@javierggt
Copy link
Contributor Author

ha, yes. I thought Github would have figured that out.

@javierggt javierggt merged commit 6184470 into master Apr 16, 2024
1 check passed
This was referenced Apr 17, 2024
@javierggt javierggt mentioned this pull request May 1, 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.

3 participants