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

Regular grid interpolation #244

Merged
merged 25 commits into from
Jun 28, 2023
Merged

Conversation

flabowski
Copy link
Contributor

see #243

The class needs a grid, i.e. something like grid = ([0.1, 0.2, 0.3], [1, 2, 3, 4, 5]), not a list of points such as points = [[0.1, 1], [0.1, 2], [0.1, 3], ..., [0.3, 5]]. That means rom.fit() does not work, the method assumes we want to use the points from the database to construct the interpolator. Instead the user has to call reduction.fit(...) first and then rg_approximation.fit(...) with the proper grid.
Although the interpolation returns what we expect in the tests, I have not tested it in a full ROM yet.

@mtezzele
Copy link
Contributor

Hi @flabowski. What about adding a preprocessing step within this class to create the 1d vectors from the parameters given by the Database class? The check on the inputs should raise an error if there are not on a grid, and depending on the dimensions create the vectors to feed the RegularGridInterpolator. What do you think?

ezyrb/regular_grid.py Outdated Show resolved Hide resolved
@flabowski
Copy link
Contributor Author

Hi @mtezzele
it would be much easier if the user would supply that directly. But yes, it makes sense to figure out that mapping, so the grid interpolation fits into ezyrb seamlessly.
It requires some index magic, because we dont know how the user aranged the points and values ('xy' or 'ij' indexing or even something random)...

@flabowski flabowski requested a review from mtezzele June 23, 2023 12:45
Copy link
Contributor

@mtezzele mtezzele left a comment

Choose a reason for hiding this comment

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

Could you please add a doc file for the automatic doc generation of the new module. You can follow what has been done in docs/source/rbf.rst for example. Then just add the name of the new file here: docs/source/code.rst

ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
tests/test_regular_grid.py Outdated Show resolved Hide resolved
Copy link
Member

@fandreuz fandreuz left a comment

Choose a reason for hiding this comment

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

Hi @flabowski, I left some comments on your code. Let me know if you need any clarification. Thanks!

ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
ezyrb/regular_grid.py Outdated Show resolved Hide resolved
@flabowski
Copy link
Contributor Author

flabowski commented Jun 26, 2023

Hei @mtezzele and @fandreuz,
Thank you for your feedback, great to see that there is some interest in this PR :) I think i addressed all of your comments, let me know in case I missed something.

@flabowski
Copy link
Contributor Author

I realized now that one of the changes @fandreuz suggested changed the behaviour and one of the tests failed. I assume that was unintended. I reverted it, so the behaviour matches the Linear interpolator again. Also, I introduced a simplification, which should not change the output.

Copy link
Contributor

@mtezzele mtezzele left a comment

Choose a reason for hiding this comment

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

Please add the new rst doc file in the following file: docs/source/code.rst

:return: the interpolated values.
:rtype: numpy.ndarray
"""
new_point = np.asarray(new_point)
if len(new_point.shape) == 1:
new_point.shape = (-1, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

please fix this as above

@ndem0
Copy link
Member

ndem0 commented Jun 27, 2023

Dear @flabowski, thanks for the effort! From my side, everything looks almost perfect, I just asked if you can add a simple test in order to check that the exception is raised in case of not grid points.

@@ -10,10 +10,10 @@ RegularGrid
:nosignatures:

RegularGrid
RegularGrid.def
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed. Or at least we don't use it in the other files. I am not sure about the generated outcome...

Copy link
Contributor Author

@flabowski flabowski Jun 28, 2023

Choose a reason for hiding this comment

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

I am not sure either. The file was generated automatically when ran make_rst.sh. Some other things seem to be fishy.

edit: everything seems fine after reverting what make_rst.sh did. @ndem0 maybe some commands need to be updated?

Copy link
Member

Choose a reason for hiding this comment

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

We are not using that script anymore! Sorry, it's not written, I gonna remove it in the next release of EZyRB (#232).
The file now looks correct, thanks!

@ndem0 ndem0 merged commit 496b071 into mathLab:master Jun 28, 2023
flabowski added a commit to flabowski/EZyRB that referenced this pull request Sep 14, 2023
* added tests for regular grid interpolation
* added regular grid interpolation module
* added the rst file for the regular grid interpolation
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.

4 participants