move some functions from create_derived_agasc_h5.py #161

javierggt · 2023-12-13T20:59:20Z

Move some functions from create_derived_agasc_h5.py into the agasc module.

The objective is to be able to call agasc.write_agasc from my AGASC-Gaia scripts.

Description

Fixes #

Interface impacts

Testing

Unit tests

No unit tests
Mac
Linux
Windows

Independent check of unit tests by [REVIEWER NAME]

[PLATFORM]:

Functional tests

from agasc import write_agasc
from pathlib import Path
h5 = tables.open_file(Path(os.environ['SKA']) / 'data' / 'agasc' / 'agasc_1p7.h5')
t = h5.root.data[:1000]
write_agasc('test.h5', stars=t, version='1p9')

…dule

javierggt · 2023-12-14T17:38:12Z

I should have marked this as WIP. I just added another commit.

One question that is left is whether this is the right place for this. The stuff I'm adding might be better in a separate utils submodule.

taldcroft

Looks generally good. In additional to the inline comments, all the public functions need full doc strings. The non-public ones should at least have a single-line docstring with a simple description of what it is doing.

taldcroft · 2023-12-27T12:12:44Z

agasc/agasc.py

+        h5.root.healpix_index.attrs["nside"] = nside
+
+
+DTYPE = np.dtype(


Make this more descriptive like TABLE_DTYPE.

taldcroft · 2023-12-27T12:14:06Z

agasc/agasc.py

+)
+
+
+class Order(Enum):


More descriptive, like TableOrder.

taldcroft · 2023-12-27T12:16:35Z

agasc/agasc.py

+    if stars.dtype != DTYPE:
+        cols = DTYPE.names
+        missing_keys = set(cols) - set(stars.dtype.names)
+        assert not missing_keys, f"Missing keys in stars: {missing_keys}"


No assert in non-test modules. Do a test and raise a ValueError instead. FWIW running Python with the -OO flag strips out assert statements. Not that we do that, but I've just been reviewing a monster astropy PR that makes astropy work with -OO.

Done.

Note: there was a trivial assert right after this, and I just removed it. My tendency would have been to keep this second assert, and let it be stripped by the -OO option.

taldcroft · 2023-12-27T12:20:20Z

agasc/agasc.py

+        write_healpix_index_table(filename, healpix_index, nside)
+
+
+def write_agasc_(filename, stars, version):


Private methods have a leading underscore, not trailing.

taldcroft · 2023-12-27T12:23:45Z

agasc/tests/test_agasc_2.py

+    from agasc import write_agasc
+
+    with (
+        tempfile.TemporaryDirectory() as tempdir,


Use the pytest tmp_path fixture. https://docs.pytest.org/en/7.1.x/how-to/tmp_path.html

taldcroft · 2023-12-27T12:24:41Z

agasc/tests/test_agasc_2.py

+            Path(os.environ["SKA"]) / "data" / "agasc" / "agasc_1p7.h5"
+        ) as h5_in,
+    ):
+        t = Table(h5_in.root.data[:1000])


This is the only line that needs the h5_in context, so out-dent all the rest to take them out of the context manager.

taldcroft · 2023-12-27T12:38:41Z

agasc/agasc.py

+    if order == Order.DEC:
+        logger.info("Sorting on DEC column")
+        idx_sort = np.argsort(stars["DEC"])
+    elif order == Order.HEALPIX:
+        logger.info(
+            f"Creating healpix_index table for nside={nside} "
+            "and sorting by healpix index"
+        )
+        healpix_index, idx_sort = get_healpix_index_table(stars, nside)


This is an opportunity to use the new switch statement in Python 3.10!

As far as I understand it, the switch statement gains us nothing in terms of performance, and it actually makes the code slightly more verbose, so I'm not sure we want to use it.

Anyway, I changed it.

…rything out of pytables context, and renamed table with one-letter name.

taldcroft

Looks good, nice!

jeanconn · 2024-01-08T18:34:17Z

agasc/tests/test_agasc_2.py

+    from agasc import write_agasc
+
+    with tables.open_file(
+        Path(os.environ["SKA"]) / "data" / "agasc" / "agasc_1p7.h5"


This is now failing on HEAD because the file in /proj/sot/ska/data/agasc is agasc1p7.h5 (no _). I think.

I happen to have a link with the _. I don't know why.

Yeah, I couldn't remember the filename history to know if we wanted to make the link you have or update the test to match what we've got in /proj/sot/ska.

javierggt requested a review from taldcroft December 13, 2023 20:59

move some functions from create_derived_agasc_h5.py into the agasc mo…

a986304

…dule

javierggt force-pushed the write-agasc branch from d551ff1 to a986304 Compare December 13, 2023 21:03

javierggt added 3 commits December 13, 2023 16:05

fixup

29b649a

fixup

82330e4

fixup

447d4a6

javierggt force-pushed the write-agasc branch from 303d708 to 4b4e2ab Compare December 14, 2023 17:36

javierggt force-pushed the write-agasc branch 2 times, most recently from 17dae1c to 8c5e834 Compare December 14, 2023 17:41

make sure agasc.write_agasc outputs the correct dtype, and add test

9160682

javierggt force-pushed the write-agasc branch from 8c5e834 to 9160682 Compare December 14, 2023 17:45

taldcroft reviewed Dec 27, 2023

View reviewed changes

javierggt added 7 commits January 4, 2024 11:05

test_agasc_2.py::test_write: use tmp_path fixture, outdent almost eve…

a5edbd0

…rything out of pytables context, and renamed table with one-letter name.

test_agasc_2.py::write_agasc: use switch statement

250206d

rename write_agasc_ -> _write_agasc

2cf2821

remove assert from agasc.write_agasc

30dd3a1

rename DTYPE -> TABLE_DTYPE and Order -> TableOrder

7bf9a1b

black

2ff8f69

ruff

bb15134

javierggt requested a review from taldcroft January 4, 2024 16:23

javierggt added 3 commits January 4, 2024 13:26

add docstrings

8c81bea

docstring fixup

c948d88

black

5bd253a

taldcroft approved these changes Jan 5, 2024

View reviewed changes

javierggt merged commit b457c13 into master Jan 5, 2024
1 check passed

jeanconn reviewed Jan 8, 2024

View reviewed changes

jeanconn mentioned this pull request Jan 9, 2024

Test failing on HEAD - missing agasc_1p7.h5 file #163

Closed

javierggt deleted the write-agasc branch January 9, 2024 21:59

javierggt mentioned this pull request Jan 11, 2024

2023.10 sot/skare3#1242

Merged

javierggt mentioned this pull request Feb 6, 2024

2023.11 sot/skare3#1246

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move some functions from create_derived_agasc_h5.py #161

move some functions from create_derived_agasc_h5.py #161

javierggt commented Dec 13, 2023 •

edited

Loading

javierggt commented Dec 14, 2023

taldcroft left a comment

taldcroft Dec 27, 2023

javierggt Jan 4, 2024

taldcroft Dec 27, 2023

javierggt Jan 4, 2024

taldcroft Dec 27, 2023

javierggt Jan 4, 2024

taldcroft Dec 27, 2023

javierggt Jan 4, 2024

taldcroft Dec 27, 2023

javierggt Jan 4, 2024

taldcroft Dec 27, 2023

javierggt Jan 4, 2024

taldcroft Dec 27, 2023

javierggt Jan 4, 2024

taldcroft left a comment

jeanconn Jan 8, 2024

javierggt Jan 8, 2024

javierggt Jan 8, 2024

jeanconn Jan 8, 2024

		h5.root.healpix_index.attrs["nside"] = nside


		DTYPE = np.dtype(

		write_healpix_index_table(filename, healpix_index, nside)


		def write_agasc_(filename, stars, version):

		)


		class Order(Enum):

move some functions from create_derived_agasc_h5.py #161

move some functions from create_derived_agasc_h5.py #161

Conversation

javierggt commented Dec 13, 2023 • edited Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

javierggt commented Dec 14, 2023

taldcroft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taldcroft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javierggt commented Dec 13, 2023 •

edited

Loading