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 skip_zero_time property in OpenFOAMReader #2847

Merged
merged 7 commits into from
Jun 26, 2022

Conversation

Failxxx
Copy link
Contributor

@Failxxx Failxxx commented Jun 22, 2022

Overview

Expose the SkipZeroTime property for OpenFOAMReader.

Resolves #2843

@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Merging #2847 (2cca83b) into main (5f3da83) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2847   +/-   ##
=======================================
  Coverage   94.03%   94.03%           
=======================================
  Files          76       76           
  Lines       16399    16409   +10     
=======================================
+ Hits        15420    15430   +10     
  Misses        979      979           

Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far

pyvista/utilities/reader.py Show resolved Hide resolved
tests/utilities/test_reader.py Show resolved Hide resolved
@Failxxx
Copy link
Contributor Author

Failxxx commented Jun 23, 2022

Edit

But now I see that pipeline does not fail... I may have done something wrong in my tests...
I am able to reproduce this on another computer.


Hum.
It seems it is not enough to use _update_information.
I have to call SetRefresh (see in docs, c++ header file, c++ file) to really take this new information into account, but I do not really understand why...

Here is my test:

from pyvista import OpenFOAMReader

# First test, with `SetRefresh`

file_1 = OpenFOAMReader(
    "/home/hnum/OLART/Docs/Data/OpenFOAM/Chanson_2004_2D_am_calcul_large_iso_density/foam.foam"
)

file_1.skip_zero_time = True
print(file_1.skip_zero_time)
print(file_1.time_values)

file_1.reader.SetRefresh()

file_1.skip_zero_time = False
print(file_1.skip_zero_time)
print(file_1.time_values)

# Second test, without `SetRefresh`

file_2 = OpenFOAMReader(
    "/home/hnum/OLART/Docs/Data/OpenFOAM/Chanson_2004_2D_am_calcul_large_iso_density/foam.foam"
)

file_2.skip_zero_time = True
print(file_2.skip_zero_time)
print(file_2.time_values)

file_2.skip_zero_time = False
print(file_2.skip_zero_time)
print(file_2.time_values)

This script gives the following output:

True
[0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1.0, 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 2.0]
False
[0.0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1.0, 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 2.0]
True
[0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1.0, 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 2.0]
False
[0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1.0, 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 2.0]

@MatthewFlamm
Copy link
Contributor

Edit

But now I see that pipeline does not fail... I may have done something wrong in my tests... I am able to reproduce this on another computer.

I do not fully understand what the differences are between some of these update and refresh methods do. SetRefresh doesn't seem to work for me though. I tested this on 8d0e3c7, i.e. before you added the call to self._update_information to the setter.

import pyvista
from pyvista import examples

filename = pyvista.download_cavity(load=False)
reader = pyvista.get_reader(filename)
reader.time_values  # Has 0.0 in it

reader.skip_zero_time = True
reader.time_values  # Has 0.0 in it

reader.reader.SetRefresh()
reader.time_values  # Has 0.0 in it

reader._update_information()
reader.time_values  # No longer contains 0.0

@Failxxx
Copy link
Contributor Author

Failxxx commented Jun 24, 2022

Here is another test. I am using the same configuration for the OpenFOAMReader as in 8d0e3c7.
SetRefresh() seems to be doing something that _update_information does not do.
Could you please confirm that you are also able to reproduce this?

import pyvista

filename = "/home/hnum/OLART/Docs/Data/OpenFOAM/cavity/foam.foam"
reader = pyvista.get_reader(filename)

reader.skip_zero_time = False
reader._update_information()
print(reader.skip_zero_time is False)   # True
print(0.0 in reader.time_values)        # True

reader.skip_zero_time = True
reader._update_information()
print(reader.skip_zero_time is True)    # True
print(0.0 not in reader.time_values)    # True

reader.skip_zero_time = False
reader._update_information()
print(reader.skip_zero_time is False)   # True
print(0.0 in reader.time_values)        # False <--- Should be True here

reader.skip_zero_time = True
reader._update_information()
print(reader.skip_zero_time is True)    # True
print(0.0 not in reader.time_values)    # True

reader.skip_zero_time = False
reader._update_information()
print(reader.skip_zero_time is False)   # True
print(0.0 in reader.time_values)        # False <--- Should be True here

reader.skip_zero_time = True
reader._update_information()
print(reader.skip_zero_time is True)    # True
print(0.0 not in reader.time_values)    # True

reader.reader.SetRefresh()

reader.skip_zero_time = False
reader._update_information()
print(reader.skip_zero_time is False)   # True
print(0.0 in reader.time_values)        # True <--- Added setRefresh(), it seems to be working

reader.skip_zero_time = True
reader._update_information()
print(reader.skip_zero_time is True)    # True
print(0.0 not in reader.time_values)    # True

reader.skip_zero_time = False
reader._update_information()
print(reader.skip_zero_time is False)   # True
print(0.0 in reader.time_values)        # False <--- Should be True here

@MatthewFlamm
Copy link
Contributor

Here is another test. I am using the same configuration for the OpenFOAMReader as in 8d0e3c7.
SetRefresh() seems to be doing something that _update_information does not do.
Could you please confirm that you are also able to reproduce this?

Good investigating! This is puzzling that one method is required in one direction yet a different method is required in the opposite direction. Would you recommend adding SetRefresh into _update_information, or just calling it in addition only in this setter?

@tkoyama010 tkoyama010 added the enhancement Changes that enhance the library label Jun 24, 2022
@Failxxx
Copy link
Contributor Author

Failxxx commented Jun 24, 2022

Would you recommend adding SetRefresh into _update_information, or just calling it in addition only in this setter?

Hard to tell haha. Since we do not fully understand what these methods do, I think we should only add SetRefresh() in this setter to avoid any problem caused by adding it to _update_information().

I am going to do some more testing soon.

@Failxxx
Copy link
Contributor Author

Failxxx commented Jun 25, 2022

Ok so I've tested with this version (SetRefresh() in the setter) and it seems to be working as intended. Do you think we can implement it this way for the moment?

@Failxxx Failxxx marked this pull request as ready for review June 25, 2022 09:38
Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tkoyama010 tkoyama010 enabled auto-merge (squash) June 25, 2022 20:14
@tkoyama010 tkoyama010 disabled auto-merge June 25, 2022 21:34
@akaszynski akaszynski merged commit 257f4aa into pyvista:main Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Changes that enhance the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose SkipZeroTime property of vtkOpenFOAMReader
4 participants