Typing: Add typehinting for Rod modules

[ankith26]

This PR does some initial work on #255

This is a WIP, and I've opened this PR to get some feedback, before I go ahead and look into more modules.

[skim0119]

@ankith26 I allowed the github-action to run for you to check. For formatting, you can use make formatting or setup-up a pre-commit hook.

[skim0119]

@armantekinalp Since we are bumping up to 3.10, lets see if we can pass this PR and add mypy checker.

[armantekinalp]

@ankith26 if you are still interested in this issue can you resolve the conflicts please.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (update/mypy@4d05804). Learn more about missing BASE report.

❗ Current head 4ec414a differs from pull request most recent head 856e901

Please upload reports for the commit 856e901 to get more accurate results.

Additional details and impacted files

@@              Coverage Diff               @@
##             update/mypy     #341   +/-   ##
==============================================
  Coverage               ?   87.80%           
==============================================
  Files                  ?       43           
  Lines                  ?     2943           
  Branches               ?      341           
==============================================
  Hits                   ?     2584           
  Misses                 ?      335           
  Partials               ?       24           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ankith26]

I have force pushed with the conflicts fixed. The conflicts were mainly due to the fact that I was adding type hints to the functions that were being deprecated in the update-0.3.3 branch. I removed my changes in these cases

[armantekinalp]

@ankith26 thanks. Can you also see why the CI builds fail?

[ankith26]

While resolving the conflicts I forgot to remove an import that is now unneeded. I have removed that import to fix the fail, and also made sure make autoflake8-check passed locally

[skim0119]

@ankith26 Hi -- I added mypy on GitHub action, and, as we have somewhat expected, it is becoming a large job to finish. We have decided that we should split the work on #255, and we can probably pass this PR without completely checking out all CIs.

Thank you again for your contribution. If you can finish typehinting for elastica/rod directory, we'll merge this PR with #367. You can run mypy --config-file pyproject.toml elastica/rod

[ankith26]

If you can finish typehinting for elastica/rod directory, we'll merge this PR with #367.

Sure I'd like to work on this this

[ankith26]

Update: I couldn't do much on this PR in the past week due to being busy with other commitments, but I plan to get this PR done from my side in the coming week.

[armantekinalp]

@ankith26 no worries, also please see the other open PRs related to mypy. That could be useful for you.

[ankith26]

A couple of notes

• mypy considers np.floating and builtin float incompatible. This is causing trouble in a couple of places.
• mypy also doesn't like the usage of numba decorators, as they apparently don't preserve typing info.

Any suggestions on how to deal with above issues?

[ankith26]

I have implemented a workaround for the float issue. In elastica.typing I have defined Float: TypeAlias = Union[np.floating, float], and now this new type can be used where a single float is expected

[skim0119]

I have implemented a workaround for the float issue. In elastica.typing I have defined Float: TypeAlias = Union[np.floating, float], and now this new type can be used where a single float is expected

In this PR, please just use np.floating whenever you need to type float type for now, and don't create another type alias. We'll make that decision later.

mypy also doesn't like the usage of numba decorators, as they apparently don't preserve typing info.

The function should be typed as if they are just python/numpy function. I understand that
numba decorator will change the function call so mypy would raise error. For now, just ignore them.

[ankith26]

I have done some more incremental progress, and have removed the Float type as you asked.

[armantekinalp]

Lets keep in draft until it is ready. Since it burns CI hours for every new commit

[ankith26]

This PR is nearing completion. Though there are a couple of issues that I don't know how to resolve.

1. The following attributes are expected in a SystemType instance in various places across the codebase, but not typed into SystemType. How should these be dealt with?
   periodic_boundary_nodes_idx
periodic_boundary_elems_idx
director_collection
radius
mass
tangents
director_collection
velocity_collection
internal_forces
external_forces
internal_torques
origin
normal
lengths
length
ring_rod_flag
n_elems

2. Similarly, RodType is used in a couple of places where these attributes are expected:
   velocity_collection
dilatation
n_elems

3. RodType incompatible with SystemType error, what is the actual relation between the two and how can this be fixed in typing?

4. ConstraintBase: Why are properties constrained_position_idx and constrained_director_idx typed with optional?

5. How to type a MemoryBlock in cosserat_rot.py

6. KnotTheoryCompatibleProtocol defines director_collection and radius read-only, but value set in cosserat_rot.py

7. CosseratRod instance expects the following attributes, but these aren't set in the constructor:
   ghost_elems_idx
ghost_voronoi_idx

[skim0119]

Thank you for your hard work. There are some inconsistency because the original code was only developed for Rod implementation, and later we decided to add other types like RigidBody and Surfaces. I'll take care of organizing properties in other PR.

To answer your question:

1. The following attributes are expected in a SystemType instance in various places across the codebase, but not typed into SystemType. How should these be dealt with?

SystemType should include all Rod, RigidBody, and Surface, so please use it whenever these objects needs to be passed or returned. For now, don't worry about any mypy error caused by missing property.

2. Similarly, RodType is used in a couple of places where these attributes are expected:

Same as above. We'll have SystemType which is union of RodType, RigidBodyType, and SurfaceType. If you need to type-hint Rod specific module, use RodType.

3. RodType incompatible with SystemType error, what is the actual relation between the two and how can this be fixed in typing?

Don't worry about it for now.

4. ConstraintBase: Why are properties constrained_position_idx and constrained_director_idx typed with optional?

There are some boundary condition implementations that do not use _idx in the definition. You don't need to modify at this point.

5. How to type a MemoryBlock in cosserat_rot.py

Use RodType.

6. KnotTheoryCompatibleProtocol defines director_collection and radius read-only, but value set in cosserat_rot.py

The protocol for KnotTheory shouldn't write to those properties. If you see any error, I would assume it is because RodType is not fully defined yet.

7. CosseratRod instance expects the following attributes, but these aren't set in the constructor:

You don't need to worry about it, it will be included in RodType in the later commit.

[ankith26]

This PR is now done from my side, it is ready for review.

[skim0119]

Looks good. One minor change, and we can probably merge.

[ankith26]

I had added that bit to fix some mypy issues, but didn't realise it broke the unit tests. I have reverted that change, as suggested.