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 python version the set of functions of cell integral and their test functions #420

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

RunxinNi
Copy link
Contributor

@RunxinNi RunxinNi commented Dec 15, 2019

This PR is to add the python version for the cell integral and cell averages functions which are originally written in Fortran. Also, add the test function to test these functions in python.

@rjleveque
Copy link
Member

Can you please add some description to this PR to explain the use case you have in mind for this? Also the doc string in the function is not very informative so I'm not sure how I'd use it.

One thing that would be useful in some cases is to be able to provide a set of topofiles, (or topotools.Topography objects in Python) and a grid patch and to be able to compute the GeoClaw cell averaged values of topo in Python that agree exactly with what the Fortran code would compute. This could be handy if trying to see how the topofiles provided are going to be translated into topo values on a fine grid that doesn't appear until later in the GeoClaw simulation, e.g. to determine if a sea wall or dike will give a barrier once discretized. But I can't tell whether this is what you have in mind or if what you have could be adapted to that purpose?

Also I see your test problem generates random data. For a unit test I think it would be better to do something deterministic and check against stored values or known cell averages for a simple function? And at the moment the tests are not passing due to being called with the wrong number of arguments?

@mandli
Copy link
Member

mandli commented Dec 16, 2019

@RunxinNi can you respond to this?

@RunxinNi
Copy link
Contributor Author

Hi Kyle, sure, prepared for today morning's final yesterday. Sorry for delaying response.
Dear Prof. LeVeque, I will add more information for these docstrings and detailed description for this PR.
Yes, it is what I have in mind. Provide a set of topofiles, (objects in Python) and a grid patch and to be able to compute the GeoClaw cell averaged values of topo in Python that agree exactly with what the Fortran code would compute.
For test_integral, for every test, it will generate a specific set of data by a known simple function which is the argument func of test_integral. Then, test_integral will use them with cell integral functions to calculate every cell averages. Finally, test_integral will compare cell averages calculated by the cell integral functions with known cell averages for a simple function func. For random I use here, every time using this function, it may generate different sets of data for testing. What I have in mind is to make sure test_integral works well for many cases not for a specific set of data. I'm not sure what potential problems with random here. Could you kindly give me some suggestions?
About "tests are not passing due to being called with the wrong number of arguments", these two test functions work well in my laptop. I will check why it doesn't work well with nosetests.

Update the docstring. Put the  functions outside the Topography class.
Update the test function with deterministic data. Test the cell integral and cell averages with continuous and discontinuous functions.
@RunxinNi
Copy link
Contributor Author

RunxinNi commented Dec 20, 2019

Add the PR description.
Update the set of functions for cell integral and cell average. Revise the docstrings. Put functions outside the Topography class.
Update the test function with the set of deterministic data. Test the cell integral and cell averages functions with continuous and discontinuous functions.
They seem passing the test.

@mandli
Copy link
Member

mandli commented Dec 26, 2019

This looks good to me save for a warning that is popping up in the tests:

testdiscontinuous is used to test discontinuous function with `integral_topotool`. ... /home/travis/virtualenv/python2.7.15/lib/python2.7/site-packages/scipy/integrate/quadpack.py:860: IntegrationWarning: The maximum number of subdivisions (50) has been achieved.
719  If increasing the limit yields no improvement it is advised to analyze 
720  the integrand in order to determine the difficulties.  If the position of a 
721  local difficulty can be determined (singularity, discontinuity) one will 
722  probably gain from splitting up the interval and calling the integrator 
723  on the subranges.  Perhaps a special-purpose integrator should be used.
724  **opt)

Do you see this locally as well?

return topoint


class patch:
Copy link
Member

Choose a reason for hiding this comment

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

Given the duplicate name in PyClaw and the fact that this is simply a data container can we change this to a dictionary?

@RunxinNi
Copy link
Contributor Author

Hi Kyle, I fixed the warning issue.

@mandli
Copy link
Member

mandli commented Dec 26, 2019

Can you also change patch to be a dictionary being passed through the chain rather than an object?

@RunxinNi
Copy link
Contributor Author

Hi Kyle, I change patch to be a dictionary.

@mandli
Copy link
Member

mandli commented Dec 29, 2019

Thanks @RunxinNi. @rjleveque does this look good to you now?

@rjleveque
Copy link
Member

I started trying to test this and ran into unexpected results in the simple case where I defined a small topo array and a patch that was exactly aligned with that array, so I expected cell_average_patch to return an array identical to the topo array. But it didn't, so either I don't understand how to use it or there's a bug.
test_cell_average_patch.pdf

@RunxinNi
Copy link
Contributor Author

RunxinNi commented Jan 11, 2021

Dear Prof. LeVeque, I detected the bug associated with a function. I fixed it. I re-did the test. Here is the result. I think there is a little issue about defining the topo. I will work with Kyle about it and give you updates later.
redo_test_cell_average_patch.pdf

@rjleveque
Copy link
Member

@RunxinNi - Yes, you are right that I defined my topo data incorrectly to obtain what I was expecting to see. Since the topo values are assumed to be pointwise values and the cell averages are computed by averaging the resulting piecewise bilinear function, you are right that you need to define the topo values at cell corners in order for the resulting cell average to have the the expected value from the same linear test function I used.

I would like to do some more testing now with overlapping topo files at different resolutions.

We're trying to get v5.8.0 out soon since that has some important bug fixes, so I might not get to this right away, but will try to soon. This will be a very useful capability to have in Python and will assist in setting up grid extents and resolutions for geoclaw runs, so thanks for working on this.

@RunxinNi
Copy link
Contributor Author

@rjleveque It's my pleasure to contribute to GeoClaw. Look forward to your further suggestions.

@mandli
Copy link
Member

mandli commented Jun 21, 2021

Just realized that this PR is still in the waiting. @rjleveque have you had a chance to look at this again?

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.

3 participants