maroba / findiff

Python package for numerical derivatives and partial differential equations in any number of dimensions.
MIT License
420 stars 60 forks source link

Non-uniform grids in 0.6.0 #8

Closed duartenina closed 5 years ago

duartenina commented 5 years ago

I don't know how to use non-uniform grids in the new version (0.6.0). All examples (including in this repo) are still using the way 0.5.2 worked.

Also, in the readme, in the non-uniform section

# Define the partial derivative with respect to y, e.g.
d_dy = FinDiff(1, x)

Shouldn't it be FinDiff(1,y)?

maroba commented 5 years ago

Hi,

actually the api shouldn‘t have changed from 0.5.2 to 0.6.0 and the examples should still apply... don‘t they?

Well, but you are right, the readme is incorrect there. But it’s not only ‚y’ either. It should contain the „coords“ keyword. I will correct it tomorrow after work.

You can find some non-uniform examples in the test folder in the test_findiff module.

If you have any questions or notice bugs, please let me know. :-)

duartenina commented 5 years ago

Using the examples found in the readme and /examples, the same error appears (in 0.6.0):

Traceback (most recent call last):
  File "test2.py", line 58, in <module>
    d_dx = FinDiff(0, x_nu, acc=2)
  File "C:\Programs\Anaconda3\lib\site-packages\findiff\findiff.py", line 21, in __init__
    self.root = PartialDerivative(*args)
  File "C:\Programs\Anaconda3\lib\site-packages\findiff\findiff.py", line 210, in __init__
    tuples = self._convert_to_valid_tuple_list(args)
  File "C:\Programs\Anaconda3\lib\site-packages\findiff\findiff.py", line 262, in _convert_to_valid_tuple_list
    self._assert_tuple_valid(t)
  File "C:\Programs\Anaconda3\lib\site-packages\findiff\findiff.py", line 273, in _assert_tuple_valid
    if h <= 0:
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

The code used in /test is working.

duartenina commented 5 years ago

OK, I tested a bit more. FinDiff still requires at least two arguments when using the coords kwarg, but it ignores the second argument (the spacing). To change the order of the derivative, we need to give FinDiff three arguments.

Code used to test it: https://gist.github.com/duartenina/d02791e2b3733d3e18ff829bc6e8b241.

EDIT: For 1D grids, it is necessary to wrap the coords in a list. The documentation is ambiguous in this case.

maroba commented 5 years ago

Thanks for the sample! I see I have mixed up the 0.5.2 API and what I tried out during development. Then I accidentally merged the experiments into the master, including their correctly running tests. ;-) I fixed that now and published a patch version 0.6.1 where the examples should run as in 0.5.2 with the same API. Could you try it, please?

duartenina commented 5 years ago

The 0.5.2 API now works correctly, but the new one doesn't. It should accept "(axis, count)" for non-uniform grids., but if you give two or three args and the coords kwarg, it will interpret the second arg as spacing and ignore the coords.

Code here.

figure_1

EDIT: This was the same behaviour as 0.5.2.

maroba commented 5 years ago

Oh, I see. I shouldn't rely on memory but look up the code history instead. ;-)

To be honest, I regret having introduced the syntax corresponding to the orange dots in your picture and actually would prefer to support only the syntax corresponding to the green one. It would make maintenance life much easier and is also more in line with the pattern for uniform grids (axis, spacing,[order]) <--> (axis, coords, [order]), where [] means optional. I'd appreciate your opinion on this. What do you think?

duartenina commented 5 years ago

I also think it's more intuitive just to replace the 'spacing' for the 'coords'. I'm not sure what would be the advantage of the other syntax.

maroba commented 5 years ago

Agreed then. I have published 0.6.2 accordingly.