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

New initialisation method when passin a df.Field using the new xarray functionality. #135

Merged
merged 11 commits into from Mar 29, 2022

Conversation

lang-m
Copy link
Member

@lang-m lang-m commented Mar 23, 2022

Test

  • field with mesh.n = (100, 100, 10)
  • creation of a new field with mesh.n = (10, 10, 10) and passing the old field to value

Performance improvement:

  • old implementation (using that the field is callable): ~ 6.5 s
  • new implementation (interpolation done by xarray): ~ 5 ms

@swapneelap This is probably the first use case for your new method.

This is required to be able to have a implementation of as_array for df.Field
without running into problems with circular imports.
@swapneelap
Copy link
Member

@lang-m Happy to see that it is useful to the project! :)
Great performance improvement BTW.

Copy link
Member

@marijanbeg marijanbeg left a comment

Choose a reason for hiding this comment

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

Looks very good! Just added a few minor suggestions.

Is there a reason why we want all _as_array in Field class and not in dfu?

discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
@lang-m
Copy link
Member Author

lang-m commented Mar 24, 2022

@marijanbeg Could you please have a look at the last test failure. This is a problem with floating-point accuracy. My suggestion would be to slightly relax region.__contains__ by adding numpy.isclose tests for pmin and pmax. This would mean going from other.pmin >= self.pmin to other.pmin > or "approx equal" self.pmin. Have you got any other ideas?

@marijanbeg
Copy link
Member

@lang-m, I agree. I am just not sure what is the right way of avoiding hard-coded tolerances. Maybe we could define some tolerances in utils and use them throughout df.

@lang-m
Copy link
Member Author

lang-m commented Mar 24, 2022

@marijanbeg Do you think sticking to the default settings for rtol and atol of np.isclose wouldn't be sufficient? Mabye with a smaller value for atol, the default is 1e-8

@lang-m
Copy link
Member Author

lang-m commented Mar 24, 2022

The new _as_array implementation depending on df.Field cannot be in dfu because we run into issues with circular imports. To make it consistent I decided to move everything to the field module as this also is the only place where we need theses functions.

@marijanbeg
Copy link
Member

Nice, I like that.

@marijanbeg
Copy link
Member

@lang-m, default value for rtol is fine. Only atol could be an issue because the values we have in the field can be in a wide range. Probably setting it to zero is fine.

argparse.FileType cannot handle subsequent errors correctly. Therefore, output
files were created even if the number of input and output files did not match.
These files were not cleaned up automatically.
@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2022

Codecov Report

Merging #135 (451cfc2) into master (f831fa2) will decrease coverage by 0.04%.
The diff coverage is 93.65%.

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
- Coverage   95.86%   95.82%   -0.05%     
==========================================
  Files          20       20              
  Lines        2106     2108       +2     
==========================================
+ Hits         2019     2020       +1     
- Misses         87       88       +1     
Impacted Files Coverage Δ
discretisedfield/util/util.py 97.97% <ø> (+1.42%) ⬆️
discretisedfield/field.py 97.47% <92.45%> (-0.40%) ⬇️
discretisedfield/ovf2vtk.py 100.00% <100.00%> (ø)
discretisedfield/region.py 98.75% <100.00%> (+0.03%) ⬆️
discretisedfield/util/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f831fa2...451cfc2. Read the comment docs.

@lang-m
Copy link
Member Author

lang-m commented Mar 28, 2022

@marijanbeg Setting atol to 0 does not work when one of the points is at (0, 0, 0) (or probably also just 0 in one direction, I didn't check that). I've now introduced a different way of calculating rtol and atol. Could you please review d789e1c.

@marijanbeg
Copy link
Member

It does exactly what it says on the tin.

if the provided field does not contain the region of the new field.
@lang-m lang-m merged commit 90ac2a5 into master Mar 29, 2022
@lang-m lang-m deleted the field-initialisation branch March 29, 2022 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants