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.

Sign up for GitHub

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

Jump to bottom

Add geometric helper functions #81

Merged
merged 7 commits into from
Jul 19, 2024
Merged

Add geometric helper functions #81

merged 7 commits into from
Jul 19, 2024

Conversation

cgohlke
Copy link
Member

@cgohlke cgohlke commented Jul 11, 2024 •
edited
Loading

Description

This PR adds geometric helper functions and addresses some issues and ideas raised during the discussion of #79, specifically #79 (comment):

  • Implement useful geometry functions as private ufuncs in the Cython module:

    • _distance_from_line
    • _distance_from_point
    • _distance_from_segment
    • _fraction_on_line
    • _fraction_on_segment
    • _intersection_circle_circle
    • _intersection_circle_line
    • _is_inside_circle
    • _is_inside_ellipse and _is_inside_ellipse_
    • _is_inside_polar_rectangle
    • _is_inside_range
    • _is_inside_rectangle
    • _is_inside_stadium, same as _is_near_segment
    • _is_near_line
    • _point_on_line
    • _point_on_segment
    • _segment_direction_and_length

  • Above ufuncs replace several helper functions from the _utils module (the tests were migrated):

    • project_phasor_to_line -> _point_on_segment and _point_on_line
    • line_from_components -> _segment_direction_and_length
    • mask_cursor -> _is_inside_circle
    • mask_segment -> _is_inside_stadium
    • circle_circle_intersection -> _intersection_circle_circle
    • circle_line_intersection -> _intersection_circle_line

  • Remove second component output array from two_fractions_from_phasor since it can easily be calculated from the first component.

  • Reimplement two_fractions_from_phasor as a thin wrapper of _fraction_on_segment.

  • Remove fractions output from graphical_component_analysis and replace the steps parameter with fractions, which can be the number of equidistant fractions or an array of fraction values.

  • Use _is_inside_circle and _is_inside_stadium ufuncs in graphical_component_analysis.

  • Improve the components tutorial:

    • add example of graphical solution for two components.
    • shorten animation code.
    • prefer PhasorPlot methods over matplotlib.
    • improve consistency.

  • Make the PhasorPlot.plot fmt parameter position only in accordance with matplotlib.

  • Add PhasorPlot.dataunit_to_point property to convert from data to point units.

  • Add option to plot labels in PhasorPlot.components.

There are still many areas for improvement, which could be discussed and/or addressed in this or future PRs:

  • The ufuncs are private, not part of the public API. The documentation could be improved if necessary.
  • The _is_* functions currently return int16 values instead of bool because of issues in Cython.
  • The _is_* functions take a mask parameter, which is currently unused. It might or might not be useful in the future when combining masks.
  • The components tutorial uses the private PhasorPlot._lines property. Instead, the PhasorPlot methods could return artists if possible, just like matplotlib functions.
  • We should review the default line properties used in PhasorPlot.components and PhasorPlot.cursors. Those methods frequently requires changing the parameter defaults.
  • The graphical_component_analysis could be split into two functions, for two and three components. That could simplify the API and documentation for two components.
  • Is there a reference for the graphical component analysis that could be added to the documentation? Is there an optimal combination of step size and radius? It seems to me that the default of a 100 steps with a radius of 0.05 is oversampling?

Release note

Summarize the changes in the code block below to be included in the
release notes:

Add geometric helper functions

Checklist

  • The pull request title, summary, and description are concise.
  • Related issues are linked in the description.
  • New dependencies are explained.
  • The source code and documentation can be distributed under the MIT license.
  • The source code adheres to code standards.
  • New classes, functions, and features are thoroughly tested.
  • New, user-facing classes, functions, and features are documented.
  • New features are covered in tutorials.
  • No files other than source code, documentation, and project settings are added to the repository.

@cgohlke cgohlke added the enhancement New feature or request label Jul 11, 2024
@cgohlke cgohlke self-assigned this Jul 11, 2024
@bruno-pannunzio bruno-pannunzio mentioned this pull request Jul 11, 2024
Merged
9 tasks
@cgohlke cgohlke mentioned this pull request Jul 11, 2024
Merged
9 tasks
@cgohlke
Copy link
Member Author

cgohlke commented Jul 11, 2024 •
edited
Loading

To demonstrate some of the ufuncs on a grid of points (see test_geometric_ufunc_on_grid in test__phasorpy.py):

ufunc

@bruno-pannunzio
Copy link
Contributor

Hi @cgohlke, I had the time to review the changes you implemented, which improve many areas that will benefit from the consistency and execution times of the optimizations you proposed. The ufuncs are an excelent incorporation to the tools available for developing other functions. I tested some of them, mainly for the cursors module in #48 and most of them can replace some of the functions, but I will raise the topic in the dedicated PR for the module. The modifications to the components tutorial are also a good improvement.

Regarding the topics you raised for discussion:

  • The ufuncs are private, not part of the public API. The documentation could be improved if necessary.

This can be a good idea, since it can be also useful for future collaborators to have a better understanding of this functions that can be of use for future implementations of higher level functions.

  • The components tutorial uses the private PhasorPlot._lines property. Instead, the PhasorPlot methods could return artists if possible, just like matplotlib functions.

I think it would be good if it returned the artist so we don't need to use PhasorPlot._lines. Also as currently the plot method doesn't return anything, I think it won't have much impact on other functions.

  • The graphical_component_analysis could be split into two functions, for two and three components. That could simplify the API and documentation for two components.

Also this might be a good idea. I think we need to discuss the whole module, regarding the planned implementations an how to divide the functionalities in functions. Also the naming of the functions of the module is something we should discuss to add cohesion.

  • Is there a reference for the graphical component analysis that could be added to the documentation? Is there an optimal combination of step size and radius? It seems to me that the default of a 100 steps with a radius of 0.05 is oversampling?

There is a paper vaguely describing the procedure, but I think it should be mentioned. Where should I add the reference to the paper? As a Note in the docstring of the function?

I am not aware of the optimization of the number of steps and radius. We don't really know what parameters SimFCS use but Leonel thinks than it uses 100 steps, But is something we can discuss next meeting with Leonel, maybe we can find an optimal number of steps based on the radius.

@cgohlke
Copy link
Member Author

cgohlke commented Jul 15, 2024

There is a paper vaguely describing the procedure, but I think it should be mentioned. Where should I add the reference to the paper? As a Note in the docstring of the function?

The numpy docstring standard has a section for references: https://numpydoc.readthedocs.io/en/latest/format.html#references. Let me know the reference and I can add it to this PR, or you can add it later.

I think it would be good if it returned the artist so we don't need to use PhasorPlot._lines. Also as currently the plot method doesn't return anything, I think it won't have much impact on other functions.

OK, I'll look into it. I remember there were reasons for not returning artists regarding typing, documentation, and noise in interactive use.

@bruno-pannunzio
Copy link
Contributor

The numpy docstring standard has a section for references: https://numpydoc.readthedocs.io/en/latest/format.html#references. Let me know the reference and I can add it to this PR, or you can add it later.

Excelent! This is the doi of the paper:

https://doi.org/10.1021/acs.jpca.9b07880

@cgohlke
Copy link
Member Author

cgohlke commented Jul 15, 2024

I added the reference for the graphical method and changed some PhasorPlot methods to return line artists.

@bruno-pannunzio
Copy link
Contributor

@cgohlke I think it would be good to have these functionalities in the main to implement them in the masks functions. I think this is ready to be merged if you also think so.

@cgohlke
Copy link
Member Author

cgohlke commented Jul 17, 2024

I think it would be good to have these functionalities in the main to implement them in the masks functions.

Yes, I'll add that to this PR.

@cgohlke
Copy link
Member Author

cgohlke commented Jul 18, 2024

I think it would be good to have these functionalities in the main to implement them in the masks functions.

Yes, I'll add that to this PR.

Hi @bruno-pannunzio : the changes to the cursors module turned out to be extensive. I decided to propose them in a different PR.

Can you review this PR and approve it if it looks OK?

@bruno-pannunzio bruno-pannunzio self-requested a review July 19, 2024 13:04
bruno-pannunzio
bruno-pannunzio approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@bruno-pannunzio bruno-pannunzio left a comment

Choose a reason for hiding this comment

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

Sure @cgohlke, I think this is ready for merge, and if the changes to be implemented are extensive it's a good idea to make them in another PR, also so we can review them more easily.

@cgohlke cgohlke merged commit cb9762e into phasorpy:main Jul 19, 2024
16 checks passed
@cgohlke cgohlke deleted the geometry branch July 19, 2024 15:37
@cgohlke cgohlke mentioned this pull request Jul 19, 2024
Merged
14 tasks
schutyb pushed a commit to schutyb/phasorpy that referenced this pull request Aug 9, 2024
@cgohlke @schutyb
Add geometric helper functions (phasorpy#81)
570e794
schutyb pushed a commit to schutyb/phasorpy that referenced this pull request Aug 9, 2024
@cgohlke @schutyb
Add geometric helper functions (phasorpy#81)
58e3c52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruno-pannunzio bruno-pannunzio bruno-pannunzio approved these changes

Assignees

@cgohlke cgohlke

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cgohlke @bruno-pannunzio