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

travel_time with order==2 has failed #1

Closed
espdev opened this issue Feb 5, 2020 · 2 comments · Fixed by #7
Closed

travel_time with order==2 has failed #1

espdev opened this issue Feb 5, 2020 · 2 comments · Fixed by #7
Labels
bug Something isn't working

Comments

@espdev
Copy link
Owner

espdev commented Feb 5, 2020

Currently, order == 2 for computing travel time in scikit-fmm (v 2019.1.30) has a numerical issue and does not work properly: scikit-fmm/scikit-fmm#28

The following code should be removed after fixing the issue in scikit-fmm:

@validator('travel_time_order')
def _check_travel_time_order(cls, v):
if v == TravelTimeOrder.second:
raise ValueError(
'Currently the second order for computing travel time does not work properly.'
'\nSee the following issue for details: https://github.com/scikit-fmm/scikit-fmm/issues/28'
)
return v

And also in the tests:

pytest.param(2, marks=pytest.mark.skip('https://github.com/scikit-fmm/scikit-fmm/issues/28')),

@espdev espdev added the bug Something isn't working label Feb 5, 2020
@jkfurtney
Copy link

This should be fixed in scikit-fmm master, can you give it a try and let me know if you find any issues? Thanks

@espdev
Copy link
Owner Author

espdev commented May 24, 2020

@jkfurtney I have checked my code from refactoring-and-docs branch with scikit-fmm from master. It works fine! All tests with order 1 and 2 were completed successfully.

Thank you!

espdev added a commit that referenced this issue Apr 14, 2021
@espdev espdev closed this as completed in #7 Apr 14, 2021
espdev added a commit that referenced this issue Apr 14, 2021
Update dependencies to fix #1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants