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

Changing usages of index(t) to a more robust isclose() #23

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

robotAstray
Copy link

@robotAstray robotAstray commented Aug 4, 2023

Summary

This PR addresses issue #11 .
The PR replaces the usage of index(t) with a more robust approach using np.isclose() in the plot_routines.py

Details and Comments

  • Replaced instances of index(t) with a new function called find_index_nearest_time_within_tolerance(), utilising np.isclose(). This change ensures more accurate and reliable time comparisons.

  • Introduced the rtol parameter to the find_index_nearest_time_within_tolerance() function, offering control over the relative precision used in time comparisons.

@haggaila
Copy link
Collaborator

haggaila commented Aug 6, 2023

Hi @robotAstray thanks for this PR.
The tests (which required some activation) show one lint error that is easy to fix (move an import statement).
In addition, please consider the following points:
The usage of math.isclose(), which seems to be incorrect according to the function's syntax.
The change will not achieve the same thing as the current code, which returns the index of the entry with time equal to t in the array by exact comparison. This should be improved to return the index the entry in the array with time closest to t.
It is also best not to set a default value for t because all values would be equally valid and there's no preferred one.
If you are interested, please submit the fixes.
Thanks again.

@robotAstray
Copy link
Author

@haggaila, thank you for the message, I will work on this and update you. Thank you.

@robotAstray robotAstray marked this pull request as draft August 9, 2023 10:13
Signed-off-by: robotAstray <[email protected]>
@robotAstray robotAstray marked this pull request as ready for review August 12, 2023 18:28
@robotAstray
Copy link
Author

@haggaila I made changes to the logic, I welcome feedback, suggestions. Thank you.
(I will add a docstring to the function shortly)

I renamed the function to something more appropriate that closely describe its behaviour.

Signed-off-by: robotAstray <[email protected]>
Signed-off-by: robotAstray <[email protected]>
@haggaila
Copy link
Collaborator

Thanks @robotAstray it looks better. I still see an issue - find_index_nearest_time_within_tolerance() may return None which will crash the invoking code. I'd say that the index of the closest entry should be returned, and in any case we would need to test the function to verify that it works as expected. Thanks.

…rance()` (#3)

* `plot_routines.py`: return index of the closest entry in `find_index_nearest_time_within_tolerance()`

Signed-off-by: robotAstray <[email protected]>

* fix docstring in plot_routines.py for `find_index_nearest_time_within_tolerance()`

Signed-off-by: robotAstray <[email protected]>

* fix docstring in  plot_routines.py for `find_index_nearest_time_within_tolerance()`

Signed-off-by: robotAstray <[email protected]>

* Create `/test/plot/__init__.py`

Signed-off-by: robotAstray <[email protected]>

* Create test_plot_routines.py to test `find_index_nearest_time_within_tolerance()`

Signed-off-by: robotAstray <[email protected]>

---------

Signed-off-by: robotAstray <[email protected]>
* remove __init__.py

Signed-off-by: robotAstray <[email protected]>

* __init__.py formatted

Signed-off-by: robotAstray <[email protected]>

---------

Signed-off-by: robotAstray <[email protected]>
@robotAstray
Copy link
Author

Hi @haggaila, I've made some changes to the code based on your feedback. I've addressed the issue regarding the function returning None and added proper handling to ensure that the index of the closest entry is always returned. Additionally, I've added comprehensive tests to cover different scenarios and ensure the correct behaviour of find_index_nearest_time_within_tolerance(). The trailing newline issue has also been fixed in test/plot/__init__.py. Please let me know if there's anything else that needs attention. Thank you!
(I also think test_plot_routines.py may be used to add tests for other functions belonging to plot_routines.py I can work on it in another PR if needed)

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.

2 participants