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

add type annotations to Simulation class #1919

Merged
merged 7 commits into from Feb 4, 2022

Conversation

oskooi
Copy link
Collaborator

@oskooi oskooi commented Jan 28, 2022

Even though Python is a dynamically typed language, type annotations (or hints) can be used for static analysis using tools such as mypy or pytype. It would be useful to eventually add type annotations to all Python classes and functions in the API. This way, we could add a static analyzer (including a linter) as a GitHub Action as part of the CI.

One anomaly with the current setup is that using the automated script generate_api_doc.py to update the user manual markdown page Python_User_Interface.md which is generated from the docstrings shows the module filename as part of the class name. For example, the Simulation constructor includes:

             boundary_layers: List[PML] = None,
             symmetries: List[Symmetry] = None,

which appears in the user manual as:

             boundary_layers: List[meep.simulation.PML] = None,
             symmetries: List[meep.simulation.Symmetry] = None,

It would be cleaner if the module filename meep.simulation did not appear. EDIT: Actually, this can be fixed by adding:

from __future__ import annotations

@stevengj
Copy link
Collaborator

stevengj commented Jan 29, 2022

As a general principle, this seems fine, but one has to be very carefully about overly restricting the types, because then you lose flexibility — Python potentially has a wide variety of types that can, for example, represent a real number (and which are convertible by SWIG to double when we call the C++ API).

@stevengj
Copy link
Collaborator

Note also that this requires Python 3.5; what's the minimum version of Python that we currently support?

@oskooi
Copy link
Collaborator Author

oskooi commented Jan 29, 2022

Note also that this requires Python 3.5; what's the minimum version of Python that we currently support?

The GitHub actions CI uses Python 3.6 and 3.9. The Conda packages for the latest version 1.22 use Python 3.7 - 3.10.

Python 3.5 was released in September 2015 which is fairly outdated.

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2022

Codecov Report

Merging #1919 (75758d9) into master (4f2b0a4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1919      +/-   ##
==========================================
+ Coverage   73.15%   73.16%   +0.01%     
==========================================
  Files          17       17              
  Lines        4924     4926       +2     
==========================================
+ Hits         3602     3604       +2     
  Misses       1322     1322              
Impacted Files Coverage Δ
python/simulation.py 76.87% <100.00%> (+0.01%) ⬆️

@oskooi oskooi changed the title add type hints to constructor arguments of Simulation class add type annotations to Simulation class Jan 29, 2022
@oskooi oskooi changed the title add type annotations to Simulation class add type annotations to Simulation class Jan 29, 2022
python/simulation.py Outdated Show resolved Hide resolved
@stevengj
Copy link
Collaborator

stevengj commented Feb 4, 2022

So int is a subtype of float in this type system?

@oskooi
Copy link
Collaborator Author

oskooi commented Feb 4, 2022

So int is a subtype of float in this type system?

Yes, that seems to be the case ever since PEP 484 which contains the typing module was introduced as part of the Python 3.5 release in September 2015.

@stevengj stevengj merged commit 95a1cbf into NanoComp:master Feb 4, 2022
@oskooi oskooi deleted the type_hints_sim_class branch February 4, 2022 18:37
@stevengj
Copy link
Collaborator

Is there a way to work with earlier Python releases? Requiring Python 3.7 for this is a bit annoying, since that is less than 4 years old. I don't mind requiring Python 3.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants