tum-pbs / PhiFlow

A differentiable PDE solving framework for machine learning
MIT License
1.39k stars 189 forks source link

About deprecation of instance checks #149

Closed Louis-Pujol closed 7 months ago

Louis-Pujol commented 7 months ago

Hello !

I have a little issue with the code:

import torch
from phi.torch import flow

random_vector_field = torch.rand(10, 10, 2) - 0.5

velocity = flow.StaggeredGrid(
    flow.tensor(random_vector_field, flow.spatial('x,y'), flow.channel('vector')),
    extrapolation=flow.extrapolation.ZERO,
    bounds=5.0
)

flow.plot(velocity)

It raises some warnings about instance checks inside plot:

FutureWarning: Instance checks on Grid are deprecated and will be removed in version 3.0. Use the methods instance.is_grid, instance.is_point_cloud, instance.is_centered and instance.is_staggered instead.
  return isinstance(self, Grid)
FutureWarning: Instance checks on PointCloud are deprecated and will be removed in version 3.0. Use the methods instance.is_grid, instance.is_point_cloud, instance.is_centered and instance.is_staggered instead.
  return isinstance(self, PointCloud)

This situation seems a bit awkward to me, instance checks are discouraged but the suggested alternatives internally use instance checks and the warnings are still raised.

The warnings come from the lines: https://github.com/tum-pbs/PhiFlow/blob/b753266c5bdc165a96d62f6b52f5069170ae81da/phi/field/_field.py#L142 https://github.com/tum-pbs/PhiFlow/blob/b753266c5bdc165a96d62f6b52f5069170ae81da/phi/field/_field.py#L148 https://github.com/tum-pbs/PhiFlow/blob/b753266c5bdc165a96d62f6b52f5069170ae81da/phi/field/_field.py#L154

A solution should be to run these lines inside a context manager when warnings are ignored. I was about to open a PR to suggest this change, however as the behavior seems to change in version 3.0, this solution could be quickly obsolete.

Tell me if you think that it should be interesting to add a fix with a context manager to avoid warnings in this context, if so I can open a PR.

holl- commented 7 months ago

Hi, this is a foward compatibility warning. You don't have to worry about these being raised from inside Φ-Flow. If you want to get rid of them, you can move to 3.0 with

pip install --upgrade git+https://github.com/tum-pbs/PhiFlow@3.0

As this issue will be fixed with 3.0 anyway, I don't think it's worth fixing now.

Louis-Pujol commented 7 months ago

Hi, thanks for your answer, I'll use 3.0, didn't now it was already available.

holl- commented 7 months ago

It's not officially released yet and some things still change, but it's stable.