typing cosmology's io by nstarman · Pull Request #15917 · astropy/astropy

nstarman · 2024-01-20T19:24:19Z

~~Requires #15916 (currently based on that PR. will need a rebase when it's merged).~~

I'm looking to minimally type cosmology's I/O module. In doing the types I've noticed a number of places where improving the code will enable us to replace All annotations. Let's do code changes in followup PRs. I modified one docstring to get Mypy to be happy.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-01-20T19:24:48Z

github-actions · 2024-01-20T19:24:52Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

mhvk · 2024-01-20T22:47:26Z

Thanks, this looks like a great start. However, I realize even more than with #15916 that I'm not suited to review this: I don't understand all those underscored TypeVar and don't really have time to investigate. So, I'll unsubscribe.

neutrinoceros · 2024-01-20T23:19:19Z

I'm happy to fill in as a reviewer if it helps !

neutrinoceros

Here's a first round. This is my first time reviewing actual usage of TypeVar with covariance/contravariance (as opposed to reading PEPs for years...), so I've intentionally avoided reviewing astropy/cosmology/_io/mapping.py for now and will come back to it when my first questions are addressed. Thanks !

eerovaher

I had a quick look. Some of the questions or suggestions I have are applicable to quite a few locations and I didn't highlight all of them.

eerovaher · 2024-01-22T14:13:55Z


-def html_identify(origin, filepath, fileobj, *args, **kwargs):
+def html_identify(
+    origin: Any, filepath: Any, fileobj: Any, *args: Any, **kwargs: Any


Why are there so many parameters that are not used?

This is how the universal I/O identify functions are written. I suppose some formats need them.

As I already mentioned elsewhere, if we need these arguments but we don't really care about their type then we should be annotating them as object instead of Any. The way I interpret Any is that not all types are valid, but the valid types are too difficult to write down because of dynamical code or simply not worth the effort, whereas object means that we know for sure that any type is valid.

eerovaher

I have a few remarks and questions. It would also be a good idea to have another look at all the Any annotations and to replace them with object where reasonable.

eerovaher · 2024-02-13T14:23:16Z

    cosmo : `~astropy.cosmology.Cosmology`
        The cosmology to return.
-    *args
+    *args : Any


Do you know what the NumPy docstring convention is? Is this supposed to be Any or object?

I do not think they have specific guidance on this.

eerovaher · 2024-02-13T15:21:41Z


-def cosmology_identify(origin, format, *args, **kwargs):
+def cosmology_identify(
+    origin: str, format: str | None, *args: Any, **kwargs: object


Why is the annotation for format so specific? From the code it is clear that format could be any object and the code would still work. There's a couple more similar cases in the patch (mostly involving Literal).

From the code it is clear that format could be any object and the code would still work

While true, None and str are the only types that actually make sense here, from a user perspective (I think ?). The fact that there are no actual type enforcement at runtime doesn't seem relevant to me: it's an implementation detail, which shouldn't justify making annotations less useful.

To be honest I have trouble understanding why format exists. If I want to know if something is identifiable as a Cosmology then simply calling cosmology_identify() should be sufficient and having the option of specifying format="astropy.cosmology" doesn't seem to have any purpose.

I thought it was a requirement on the signature of functions that are passed to _UnifiedIORegistryBase.register_identifier but it doesn't actually look to be the case... I don't get it either.

The I/O in Cosmology was based on the implementation from table.

Could you point out the specific lines of code?

Sure:

astropy/astropy/io/misc/hdf5.py

Line 44 in 0df2022

def is_hdf5(origin, filepath, fileobj, *args, **kwargs):

,

astropy/astropy/io/misc/parquet.py

Line 26 in 0df2022

def parquet_identify(origin, filepath, fileobj, *args, **kwargs):

,

astropy/astropy/io/votable/connect.py

Line 18 in 0df2022

def is_votable(origin, filepath, fileobj, *args, **kwargs):

are all examples.

But none of these have a format parameter.

They are the same as "filepath". But here we are not writing, so it's "format". E.g. "parquet_identify" does a check on filepath to determine it ends in ". parquet" or "parq"

For type checking purposes a function argument annotation should be as general as possible. A more specific annotation can be justified if it is simpler but still helpful to users. In this case format: object would be more useful for type checking than format: str | None and the latter can't be justified by its helpfulness because I can't think of any use cases where I'd want to specify format, meaning that the entire argument is useless regardless of how it is annotated.

neutrinoceros

Sorry I missed your request for review for so long !

nstarman · 2024-04-28T01:33:14Z

Let's do #15852 first

github-actions · 2024-10-17T05:40:16Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

Signed-off-by: nstarman <[email protected]>

Co-authored-by: Clément Robert <[email protected]> Co-authored-by: Eero Vaher <[email protected]> Signed-off-by: nstarman <[email protected]>

neutrinoceros · 2024-10-19T07:51:53Z

Is this blocked any how ? I'm seeing some unresolved threads from @eerovaher's review but they're both marked as outdated.

eerovaher

I don't understand what many of the functions are doing (e.g. why are there so many parameters that are not used in their functions at all?), but the annotations seem to be consistent both with the docstrings and what the code seems to be doing. Hopefully the annotations will help with sorting out the messy bits.

nstarman added this to the v6.1.0 milestone Jan 20, 2024

github-actions Bot added Docs cosmology io.registry labels Jan 20, 2024

nstarman commented Jan 20, 2024

View reviewed changes

Comment thread astropy/cosmology/typing.py

nstarman force-pushed the cosmo-typing-io branch from cbea4f8 to 5c56583 Compare January 20, 2024 21:14

nstarman mentioned this pull request Jan 20, 2024

Add typing module to IO #15916

Merged

1 task

nstarman force-pushed the cosmo-typing-io branch from 5c56583 to b6bf913 Compare January 20, 2024 21:46

nstarman removed the io.registry label Jan 20, 2024

nstarman requested review from eerovaher and mhvk January 20, 2024 22:02

nstarman added the no-changelog-entry-needed label Jan 20, 2024

nstarman marked this pull request as ready for review January 20, 2024 22:33

nstarman requested a review from a team as a code owner January 20, 2024 22:33

mhvk removed their request for review January 20, 2024 22:47

neutrinoceros reviewed Jan 21, 2024

View reviewed changes

Comment thread astropy/cosmology/_io/ecsv.py

Comment thread astropy/cosmology/_io/html.py Outdated

Comment thread astropy/cosmology/_io/latex.py

Comment thread astropy/cosmology/_io/yaml.py

Comment thread astropy/cosmology/_io/model.py

eerovaher reviewed Jan 22, 2024

View reviewed changes

Comment thread astropy/cosmology/_io/ecsv.py Outdated

eerovaher reviewed Jan 22, 2024

View reviewed changes

nstarman force-pushed the cosmo-typing-io branch 2 times, most recently from 6b697fd to ebabffa Compare February 8, 2024 15:52

nstarman requested review from eerovaher and neutrinoceros February 8, 2024 15:53

eerovaher reviewed Feb 8, 2024

View reviewed changes

nstarman force-pushed the cosmo-typing-io branch 2 times, most recently from 9a9617e to b57931e Compare February 8, 2024 23:38

eerovaher reviewed Feb 13, 2024

View reviewed changes

neutrinoceros reviewed Feb 14, 2024

View reviewed changes

Comment thread astropy/cosmology/_io/cosmology.py Outdated

Comment thread astropy/cosmology/_io/ecsv.py Outdated

Comment thread astropy/cosmology/_io/mapping.py Outdated

nstarman force-pushed the cosmo-typing-io branch 2 times, most recently from 929c360 to a8844ad Compare February 15, 2024 02:22

nstarman requested review from eerovaher and neutrinoceros February 22, 2024 17:29

neutrinoceros approved these changes Feb 23, 2024

View reviewed changes

astrofrog modified the milestones: v6.1.0, v7.0.0 Apr 4, 2024

nstarman force-pushed the cosmo-typing-io branch from a8844ad to 764e772 Compare April 28, 2024 01:28

nstarman marked this pull request as draft April 28, 2024 01:33

astrofrog reviewed Apr 28, 2024

View reviewed changes

Comment thread astropy/cosmology/_io/cosmology.py

nstarman force-pushed the cosmo-typing-io branch from edc5f74 to 25f48da Compare May 19, 2024 19:53

mhvk added the typing related to type annotations label May 30, 2024

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Oct 17, 2024

nstarman marked this pull request as ready for review October 18, 2024 19:38

nstarman modified the milestones: v7.0.0, v7.1.0 Oct 18, 2024

nstarman and others added 2 commits October 18, 2024 15:39

Type the IO registrants

0da9b15

Signed-off-by: nstarman <[email protected]>

updates from review

45a669b

Co-authored-by: Clément Robert <[email protected]> Co-authored-by: Eero Vaher <[email protected]> Signed-off-by: nstarman <[email protected]>

nstarman force-pushed the cosmo-typing-io branch from 25f48da to 45a669b Compare October 18, 2024 19:39

eerovaher approved these changes Oct 22, 2024

View reviewed changes

eerovaher merged commit 5c2259b into astropy:main Oct 22, 2024

neutrinoceros modified the milestones: v7.1.0, v7.0.0 Oct 24, 2024

Uh oh!

Conversation

nstarman commented Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jan 20, 2024

Uh oh!

github-actions Bot commented Jan 20, 2024

Uh oh!

Uh oh!

mhvk commented Jan 20, 2024

Uh oh!

neutrinoceros commented Jan 20, 2024

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerovaher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerovaher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman Apr 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerovaher Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman commented Jan 20, 2024 •

edited

Loading

nstarman Apr 28, 2024 •

edited

Loading

eerovaher Apr 29, 2024 •

edited

Loading