Consider adding Python type hints

[cosinekitty]

Evaluate adding Python type hints to the Python version of Astronomy Engine.

Questions:

• Does it add value to end users by helping them avoid errors calling Astronomy Engine functions?
• Does the existing pylint step in the unit tests benefit the project by detecting a wider range of mistakes in the code?

[ris-tlp]

I don't think there are any downsides to adding this. I would like to help towards this if it's still up for doing!

[cosinekitty]

Hi @ris-tlp! This would be super helpful and most welcome if you wish to take this on.

The workflow for Astronomy Engine developers/contributors is to edit "template" source files, then execute the script generate/run. This script feeds the source templates through a code generator, mostly to insert numeric lookup tables. Then it executes all the unit tests. I mention this because sometimes people don't know this and work for hours editing the generated code, which then gets overwritten by the run script! (Painful.)

If you are using Linux or Mac OS for your development platform, please take a look at the contributor tool setup instructions. Windows is also possible for contributors, although the unit tests are not as complete; then you can take a look at these instructions instead.

You can also take a look at the GitHub Actions yml script to get an idea of how it installs tools and runs that same run script.

Before beginning any code changes, it is strongly recommended that you run the following to successful completion:

cd generate
./run

It will probably take a few iterations to resolve missing dependencies. Once you get that to succeed, then you can start making changes in generate/template/astronomy.py. When you have changes you like, execute run again like above, which will also update the generated versions of astronomy.py. You need to do this every time before committing to git and pushing, or GitHub Actions will fail the commit.

So let me know what you think. If you are still interested in moving forward, I will be happy to help with any issues you run into getting your development environment set up.

[ris-tlp]

Hi! I had no issues with getting the dependencies setup and executing the run script. I did however run into 1 test failing from 3 different languages that prevented the run script from executing till completion.

Kotlin: Convert EQJ to ECT (line 289 in source/kotlin/src/test/kotlin/io/github/cosinekitty/astronomy/Tests.kt

• <failure message="org.opentest4j.AssertionFailedError: excessive sphere_diff = 3.471167061223179E-18 AU." type="org.opentest4j.AssertionFailedError">org.opentest4j.AssertionFailedError: excessive sphere_diff = 3.471167061223179E-18 AU.

Python: Ecliptic (line 1239 in generate/test.py)

• PY Ecliptic FAIL: EXCESSIVE ANGLE DIFF = 3.006602e-18 au.

C#: EclipticTest (line 2165 in generate/dotnet/csharp_test/csharp_test.cs

• C# EclipticTest: EXCESSIVE ANGLE DIFF = 3.00659E-18 AU. ERROR(./unit_test_csharp): Error running csharp unit test

Commenting these tests out allowed me to run the script to completion. I'm not exactly sure what could be causing the issue. I'm assuming they're all the same test but I don't know too much about the codebase yet. Is this to be expected? Please let me know how I can proceed, thanks!

[cosinekitty]

Hi Omar, that's great progress! I'm not at all worried about the test failures. This sort of thing has happened before. These are very tiny floating point errors, just larger than existing builds have encountered. I suspect you are using a different compiler and/or different hardware than I and GitHub Actions use. I will update the tests in the master branch. I will post here again in about an hour or so... stay tuned!

By the way, what operating system and processor are you using?

[ris-tlp]

Ah I see, that makes sense! I'm using MacOS Monterey (12.6) and a M1.

[cosinekitty]

OK, I was guessing you would say M1! It must be the first time anyone has run the tests on that processor.

I just now pushed fixes to the unit tests to the master branch. If you discard your changes where you commented out the failing tests, and pull again, this time all the tests should pass.

[ris-tlp]

Yup that seemed to work! Although I just ran into this towards the final test check which I guess stemmed from relaxing the test ranges, sorry about the hassle!

First  file: temp/c_check.txt
Second file: temp/py_check.txt
Tolerance = 3.100e-15

            lnum                 a_value                 b_value     factor       diff  name
FAIL      201528  3.4491354552834508e+01  3.4491354552832789e+01    0.00275  4.722e-15  sky_hor_az

Score = 4.722e-15
ctest(Diff): EXCEEDED ERROR TOLERANCE.

[cosinekitty]

No hassle at all! I will fix this one too. This is helpful because I want the development tools to work on the M1, and I haven't had access to one before now.

[cosinekitty]

OK, I just pushed 73f9cb4b0d3be75698fea9a20b811bb33e33b57b which should resolve that test failure.

[ris-tlp]

Awesome! All tests pass now, thanks for being super fast with the responses! I'll get started with adding the type hints and checking them with mypy locally. I'll make a smaller PR to get your feedback on it first and then proceed with doing larger batches.

[cosinekitty]

Oh cool, I didn't know about mypy. I'm reading about it now. Currently I use pylint in the build process, and it has been helpful for finding mistakes in the code. I didn't know if pylint would analyze type hints also. If mypy is the best way to find type errors, I would like to add that to the generate/run script, and make sure it can work in GitHub Actions. But first things first... I like your approach, and I'm so glad someone is willing and able to help with this!

[cosinekitty]

Thank you very much @ris-tlp for your work on adding Python type hints! This has been merged into the master branch. I also went ahead and made changes to my custom Python markdown generator to show consistent Optional[x] types (see my remarks in your PR that is now closed).

I'm going to do a little more work before releasing it. Reading the mypy documentation, I see that there is a --strict option that requires all functions to have type annotations, plus a lot more requirements to improve static checking. I have enabled this option in my local build, and I'm working through all the resulting errors to bring type hints up to 100% coverage.

[ris-tlp]

Hey Don. Awesome! I'm sorry I couldn't help towards a 100% coverage for the reasons mentioned earlier, but I'm happy I could help towards the initial goal!

As for the issues with Optional, I would wager that because using Optional[type] directly calls Union[type, None] in the typing standard lib, inspect signature somehow extracts Union rather than Optional, but I can't be sure as to why exactly.

[cosinekitty]

That makes sense about Optional. Because I'm only doing it for a Markdown documentation generator, I think it's harmless to substitute that way.

I ended up starting with the mypy option --disallow-untyped-defs instead of --strict, because some of the errors from --strict are bonkers! The option I added requires all functions to be fully type-hinted.

Here is the change: f849c112580f5dd8aec2a3530e38c692e70bf412.

This was good because it exposed some corner cases that needed fixing. Later I will experiment with adding more strictness to see if it helps improve code quality further.

This has been a great exercise so far.

I'm also thinking about re-ordering things to reduce the number of quoted type forward declarations to a minimum.

[cosinekitty]

It turns out after doing all the other stuff, using the --strict option wasn't much extra work. I also updated the documentation generator to produce better output. This is in great shape now!